#115 check and update hazard model #151
Conversation
Ordering return periods and adding them as new bands in the netcdf
… multiband netcdfs Removal of redundant functions, integration of grid functions to save multiband netcdfs
Update tests for hazard set up
|
Please add comments on how to improve the tests for the hazard components. I still miss to improve the tests. An addition point is that for one reason, when working with geotiffs the running time of the test is huge which makes me think that there is a wrong writing of the grid data. I already checked the data structure of the outputs but I require to check the spatial distribution of the maps (using qgis) to check if it is correct. |
|
Test_hazard.py was erased in the current branch so we can erase it again in fiat_integrator. |
There was a problem hiding this comment.
Very nice work in creating such an extensive test!
I just did some small things: removed the unused imports and changed the names of the test.
hydromt_fiat/fiat.py
Outdated
| # save sfincs map as GeoTIFF | ||
| # result_list = list(sfincs_model.results.keys()) | ||
| # sfincs_model.write_raster("results.zsmax", compress="LZW") | ||
|
|
There was a problem hiding this comment.
Is this still required? Otherwise it can be removed
There was a problem hiding this comment.
I removed all the sfinc components. This is a task that can be done through hydromt sfincs. I kept "da = da * unit_conversion_factor" from here as a general check.
hydromt_fiat/workflows/hazard.py
Outdated
| @@ -61,6 +61,7 @@ def check_parameters_type( | |||
| dict | |||
| Dictionary with the parameters and list of parameters used in setup_hazard. | |||
| """ | |||
| #TODO: remove this function | |||
There was a problem hiding this comment.
Is this TODO still valid or should this function remain? What do you think?
There was a problem hiding this comment.
The function was required partially. That is why par of it is still there, but a significant part was removed in hazard.py and validation.py. This function was modified to only produce a list of parameters, the check of data was already removed.
| "returnperiod": str(da_rp), | ||
| "type": da_type, | ||
| "name": da_name, | ||
| "analysis": "event", |
There was a problem hiding this comment.
What is this attribute doing? And this one also has a return period? Did you ask Brendan to test with the netcdf that was produced? I think the name of the return period attribute was 'rp' but I'm not sure
There was a problem hiding this comment.
I will coordinate with Brandon. Attributes could be skipped in this dataset as the return period is written in the bands name of the dataset. I changed it as rp meanwhile.
| check_parameters_size(params) | ||
| check_files(params, self.root) | ||
|
|
||
| # create lists of maps and their parameters to be able to iterate over them |
There was a problem hiding this comment.
Nice that the fuction setup_hazard is shorter now!
Modifications made after Frederique's review. Hydromt sfincs functions are out of hydromt fiat now. Every dataset required for hydromt fiat can be done in advace with hydromt sfincs.
Makin hazard function explicit after Dirk review
Change of .rio for .raster hyndromt core function
Adding parameter geom when reading raster data. It requires that self.region is always available.
Removing test using sfincs_map.nc. They are not processed anymore through hydromt fiat.
|
This branch can already be merged, it addresses Dirk's comments. The only comment missing is this one: #124 (comment). We have to ensure that self.region exists to load raster data with the parameter geoms=self.region. At this moment, with the current test, it is working. I also need to improve the test, let me know what is missing. I removed the test for sfincs maps as they are not read anymore through hydromt fiat. The process of this sfincs.nc maps can be done through hydromt sfincs. |
1 similar comment
|
This branch can already be merged, it addresses Dirk's comments. The only comment missing is this one: #124 (comment). We have to ensure that self.region exists to load raster data with the parameter geoms=self.region. At this moment, with the current test, it is working. I also need to improve the test, let me know what is missing. I removed the test for sfincs maps as they are not read anymore through hydromt fiat. The process of this sfincs.nc maps can be done through hydromt sfincs. |
|
Great! I will merge this branch into fiat_integrator to have everything there. We will discuss the remaining things tomorrow and add issues for those things if required. Good work! |
The set up hazard was updated to create netcdf files with multiple bands pero return period map. The update integrates a small functionality to sorte the return period maps from the lower return period to the highest return period. Some check function as check_files functions and check_param_type were removed or modified to avoid redundancy in the data checks. This included removing functions from validation.py. Functions to create list of parameters and check their size are still in the code. Data catalog is not used anymore in the set up hazard workflow so parameter catalog_name and its connections were removed.