GridModel; HydroMT 0.9 compliant; Deprecated PCR; Ubuntu testing #211
Conversation
dalmijn
changed the title
There was a problem hiding this comment.
I only skimmed the code since I can't really meaninfully review that, but most of it seems fine. I left a couple of comments that you may or may not want to do anything with. The workflows are mostly fine except the stest stable. I'm not sure that one actually does anything (or at least not what you think it does) we might have to pair up and look at that together some time so you can walk me through it. Otherwise LGTM
There was a problem hiding this comment.
In general it is considered bad practice to keep output and/or binaries in the repository, especially if they change often, git cannot deal with blobs as efficently as other types of data. Given they are this small I think it's fine, but if it changes a lot you might want to consider moving these to git LFS. I won't repeat it, but the same comment applies to all output files in this PR
There was a problem hiding this comment.
I agree, it should be something for an overhaul of the tests.
hydromt_wflow/wflow.py
Outdated
| for lib, version in artifact_keys.items(): | ||
| warnings.warn( | ||
| "Adding a predefined data catalog as key-word argument is deprecated, " | ||
| f"add the catalog as '{lib}={version}'" | ||
| " to the data_libs list instead.", | ||
| DeprecationWarning, | ||
| stacklevel=2, | ||
| ) |
There was a problem hiding this comment.
Given that you also run super().__init__() this might lead to duplicated warnings, I'm not sure, but it would be good to double check
There was a problem hiding this comment.
Will look at that.
| @pytest.fixture() | ||
| def floodplain1d_testdata(): | ||
| data = xr.load_dataset( | ||
| join(TESTDATADIR, "floodplain_layers.nc"), | ||
| join(TESTDATADIR, SUBDIR, "floodplain_layers.nc"), | ||
| lock=False, | ||
| mode="r", | ||
| ) |
There was a problem hiding this comment.
FYI: fixtures are scoped by module not by test by default in pytest so if you modify this in any way other tests will see that modification. Recently had to find out the hard way. You can add a scope argument to the fixture to change this. Not necessary but wanted to mention it anyway.
There was a problem hiding this comment.
Also something for a tests overhaul.
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| os: ["ubuntu-latest" ] #, "macos-latest", "windows-latest"] | ||
| python-version: ['3.11'] | ||
| include: | ||
| - os: ubuntu-latest | ||
| label: linux-64 | ||
| prefix: /usr/share/miniconda3/envs/hydromt_wflow | ||
|
|
||
| name: ${{ matrix.label }} | ||
| runs-on: ${{ matrix.os }} |
There was a problem hiding this comment.
if you only test for one version an done os a matrix here is not necessary, but ti does make it easier to add those down the line. up to you whether you want to keep it.
There was a problem hiding this comment.
I like the fact that it gives me some variables to work with. but en general you're right.
| export PATH=/usr/share/miniconda3/bin:$PATH | ||
| PYTHONPYCACHEPREFIX=~/pycache mamba run -n hydromt_wflow sphinx-build ./docs ./docs/_build -b dummy |
There was a problem hiding this comment.
You don't seem to use the same caching strategy as core so the export and cache previx is not necessary here. if you want me to explain why this is necessary in core but not here ping me
There was a problem hiding this comment.
Will do. Will look at that.
.github/workflows/test_dev.yml
Outdated
| run: pip install tomli && python make_env.py test --py-version ${{ matrix.python-version }}.* | ||
|
|
There was a problem hiding this comment.
I have a slight susspiscion this is going to break since .* is a glob in bash, so it is probably going to expand to things you don't expect. I think the make_env.py in core now has a update to deal with this, if not please ping me and I'll make one. If you want to keep this patern you should wrap it in quotes at least
make code more robust for different CRS
There was a problem hiding this comment.
Looks good to me! Two things that need fixing, both related to the docs:
- Update
convert_staticmaps_to_mapstack.ipynbto reflect the new way to deal with PCRaster files -
build_sediment.ipynbcurrently fails at the part where we add layers to a wflow model. I don't directly see where it goes wrong, but should hopefully be easy to fix
Perhaps also good to note (in the changelog maybe) that these changes need hydromt v0.9.1 (that is currently not yet released).
Issue addressed
Fixes #160
Explanation
Inherent from GridModel; adjusted methods and properties
Checklist
mainWell, the important tests pass, but there is something with testing against core version 0.9 that causes issues...
Additional Notes (optional)
Add any additional notes or information that may be helpful.