Refactor threedigrid_to_ogr function #997
Conversation
|
@leendertvanwolfswinkel Could you take a look at this PR please? I couldn't manage to make it work although I think that I am very close now. Since you have more experience with this, maybe you could spot some error in my logic I hope. |
|
Had a quick look. Some comments:
Hope this helps |
|
ah thanks! that sounds better indeed. I am actually moving this way so good to hear it from you too. |
|
I still keep the old threedigrid_to_ogr function in the threedigrid_ogr.py file as the old function is still being used somewhere else in the code base for the processing algorithm. |
|
Can you please replace all usages of the function? |
hoanphungt
requested review from
benvanbasten-ns and
leendertvanwolfswinkel
and removed request for
leendertvanwolfswinkel
| @@ -99,11 +99,13 @@ def __init__( | |||
|
|
|||
| def run(self): | |||
| grid_admin = str(self.result.parent().path.with_suffix('.h5')) | |||
| grid_admin_gpkg = str(self.result.parent().path.with_suffix('.gpkg')) | |||
There was a problem hiding this comment.
Does this assume that the gridadmin.gpkg is always in the same directory as gridadmin.h5. Because that is not necessarily the case always
There was a problem hiding this comment.
it grabs the griadmin.gpkg file from the parent directory of the threedi result file. For example in here:
.../threedi_results_analysis/{name_of_the_simulation}/gridadmin.gpkg
There was a problem hiding this comment.
Hi Hoan,
I now realize that this ticket has impact on more parts of the code than I originally thought, sorry for that. Also I somehow didn't think about the crucial need for subsetting in threedigrid_to_ogr.
After you implement the suggested changes, I will functionally test all the affected tools:
- Result aggregation
- Watershed analysis
- Cross-sectional discharge
Could you please give those a try as well before passing the ticket to Ready for testing?
| attributes=attributes, | ||
| attr_data_types=attr_data_types, | ||
| include_all_threedigrid_attributes=True | ||
| ids=gauge_line_ids, |
There was a problem hiding this comment.
@leendertvanwolfswinkel is gauge_line_ids the correct value to pass into the ids argument?
|
@leendertvanwolfswinkel I refactored it again. Could you please take another look? |
… tool (#1007) * Update threedigrid_ogr.py * Update cross_sectional_discharge.py * WIP - Flowlines in watershed tool still to be fixed * Only show the correct marker when browsing result sets * No longer clear results when toggling smoothing * flake8 * Update CHANGES.rst
No description provided.