Skip to content
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

Enable simple region checking for cli #664

Merged
merged 15 commits into from
Dec 1, 2023
Merged

Enable simple region checking for cli #664

merged 15 commits into from
Dec 1, 2023

Conversation

savente93
Copy link
Contributor

@savente93 savente93 commented Nov 28, 2023

Issue addressed

Fixes #638

Explanation

Given that we only support bbox and geom regions in the export CLI, I thought it was appropriate to do the same here. We can add to this over time but then we will need a more concrete proposal for who it should look and what we should support so I think this is okay for now. Then at least people can use the most simple variants.

Checklist

  • Updated tests or added new tests
  • Branch is up to date with main
  • Tests & pre-commit hooks pass
  • Updated documentation if needed
  • Updated changelog.rst if needed

Additional Notes (optional)

Add any additional notes or information that may be helpful.

@savente93 savente93 marked this pull request as ready for review November 28, 2023 13:40
@savente93 savente93 requested a review from hboisgon November 28, 2023 13:42
@savente93 savente93 changed the title enable bbox and geom region checking for cli Enable simple region checking for cli Nov 28, 2023
@savente93
Copy link
Contributor Author

savente93 commented Nov 30, 2023

Things we want to still add:

  • Add all options for drivers, even if we can't yet check that the data type and driver combo is correct
  • Check that crs is a valid crs
  • Check that the region kind is one that hydromt supports even if we cannot validate it yet, and give better errors there
  • See what is up with the setup precip forcing on grid model not being caught

@savente93
Copy link
Contributor Author

@hboisgon I added your setup_precip_forcing as a test case and it passes here so I am not quite sure what is going on here. Perhaps this indicates that the cli is the problem and not the checking, I'll try and investigate that a bit further if I have some time left today or tomorrow, but the other points we discussed should now be fixed

@savente93
Copy link
Contributor Author

Discussed briefly with Dirk that it's better to update the data catalog so the CRS like 54009 validates more easily. Should not have any impact on users.

@savente93
Copy link
Contributor Author

I figured out what the issue was with the grid model precip example Hélène found. The validator was actually only validating the first step in the file provided, not all of them. This is now rectified and it should all work. Very good catch @hboisgon !

Copy link
Contributor

@hboisgon hboisgon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice changes! Thanks @savente93

@savente93 savente93 merged commit 0bf8e2a into main Dec 1, 2023
8 checks passed
@savente93 savente93 deleted the region-checking branch December 1, 2023 10:01
@savente93 savente93 mentioned this pull request Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Model config should be able to validate region arguments
2 participants