-
Notifications
You must be signed in to change notification settings - Fork 22
Add results component #533
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add results component #533
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the Wflow model's output handling by splitting the previous single results
component into three dedicated output components, aligning with the new flexible component structure from HydroMT core v1.
- Split results component into three separate output components for different file types
- Added output_grid, output_scalar, and output_csv components with corresponding read/write functionality
- Updated tests, documentation, and examples to reflect the new component structure
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
hydromt_wflow/wflow.py | Removed old results handling and added three new output components |
hydromt_wflow/components/init.py | Added imports for new output components |
hydromt_wflow/components/output_*.py | New component files for grid, scalar, and CSV outputs |
hydromt_wflow/utils.py | Renamed read_csv_results to read_csv_output function |
tests/ | Updated tests to use new output components instead of results |
docs/ | Updated documentation and examples to reference new component structure |
Comments suppressed due to low confidence (1)
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice work! Code and tests look good, no comments at all!
Possibly, we could consider adding a sort of aggregated results component that uses the 3 components created in this pr to be more similar to the v0 implementation for users.
But to be honest, im not a big fan, I just want to have some conversation on this PR ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job on the new output components. The code is clear, neat, and well documented. I have only two minor comments.
Yes it's just in the end the way the components are build with the filename attribute and by splitting I can do more checks and processing if needed so I opted for that option. I think also nice for the user, the old dict containing everything was a little confusing I think. |
Issue addressed
Fixes #528
Explanation
Results used to be one object (dict of xarray objects). WIth the flexible component structure, I decided to split results into one component per file:
output_grid
for the netcdf gridded output. It then inherits from the GridComponent with staticmaps as its region.output_scalar
for the netcdf scalar output. I realise we do not have a simple xarrayDataset components, only the dict of Datasets in core so I started from the base ModelComponent.output_csv
for the csv output. This one is then the DatasetsComponent of core and objects within are hydromt.vector.GeoDataArray. To be able to add georeference, the staticmaps need to be passed so I added alocations_component
attribute. It's not really the region as csv can contain outputs for 3 points withint the basin so I thought defining another name was better.Checklist
feat/migration-hydromt-core-v1
Additional Notes (optional)
In the current v0 version, csv output are converted to several netcdf files and geo-references are also added in the process. For netcdf_scalar, we only read the file directly but in a way it could also be split to several GeoDataArrays in the same way as the csv output. Maybe still something we would like to do?