Add hypothesis strategy for SGRID dataset generation#2634
Conversation
7110e63 to
3d7ecaf
Compare
3d7ecaf to
b7883ef
Compare
erikvansebille
left a comment
There was a problem hiding this comment.
Looks good; just two questions below
| try: | ||
| import hypothesis # noqa: F401 | ||
| except ImportError as err: | ||
| err.add_note( | ||
| "To use strategies you must have hypothesis installed. Install it from PyPI, Conda, or using your preffered package manager." | ||
| ) | ||
| raise err |
There was a problem hiding this comment.
Why is this needed? Wouldn't hypothesis simply be part of our Pixi install?
There was a problem hiding this comment.
Its part of our Pixi install, but its not a Parcels run dependency (i.e., its not in pixi.toml::run-dependencies or the recipe.yaml/pyproject.toml.
This is our first "optional dependency" for Parcels (i.e., a part of the codebase that needs a specific package in order to fulfill a function, but where it doesn't make sense to include it for everyone since most people wont use the specific function).
People doing conda install parcels then import parcels._strategies will encounter this more informative error message.
There was a problem hiding this comment.
OK, I see. But then (in a next PR?) fix the type-o preffered to preferred?
There was a problem hiding this comment.
Perhaps this is just good practice, but I'm surprised that the strategies are moved out of the tests directory. Would they not more logically belong there?
There was a problem hiding this comment.
Yeah - good question.
Most of our dataset generation code we have shipping with Parcels via parcels._datasets. This means that users can have easy access to Parcels-compatible example datasets and dataset generation for posting issues etc. . When adding strategies for creating example datasets, I needed to use existing strategies that were in the tests module. Since I cant put code in parcels that depends on the tests module - the options I saw were:
- Leave the strategies where they are in tests and carve out a part in
tests/...for housing these dataset strategies. This would mean that some datasets are used viaparcels/_datasets/...and some viatests/...which I felt was confusing for devs - Move the dataset generation to
tests- foregoing the benefit of users having easy access to datasets - Move the strategies to
parcels
I thought (3) was the best. It might seem weird to be putting "test" code in the parcels release, but its actually not that weird - some projects even put their whole test suites in the release itself.
Description
This PR:
parcels/_datasets/structured/strategies.pyparcelsmodule (instead of thetestsmodule) -tests.strategies->parcels._strategies_datasetsfolder is in the Parcels package, I think this structure makes the most sensehypothesisinstalledThis in future will allow us to have more property based testing extending to full simulations - which would be really helpful for both improving the robustness and maintainability of the Parcels test suite.
Checklist
mainfor normal development,v3-supportfor v3 support)AI Disclosure
tests/datasets/test_strategies.py