Adding caravan forcing

[Daafip]

Hi All,

As this is my first contribution would like some feedback if possible.

Was unsure on the naming convention, generate seems to be more linked to the esmvaltool, thus went for retrieve.

Runs for me when I run:

from ewatercycle.forcing import sources
path = Path.cwd()
forcing_path = path / "Forcing"
experiment_start_date = "1997-08-01T00:00:00Z"
experiment_end_date = "2005-09-01T00:00:00Z"
HRU_id = 1022500

camels_forcing = sources['LumpedCaravanForcing'].retrieve(start_time = experiment_start_date,
                                                          end_time = experiment_end_date,
                                                          directory = forcing_path / "Camels",
                                                          basin_id = f"camels_0{HRU_id}",
                                                          variables = ('streamflow','potential_evaporation_sum'),
                                                          )

Discussions on the topic: #398

Still todo:

• remove geopandas
• remove wget
• add documentation
• add meaningfull example
• Rename variables to correct ESMValTool
• Add test (using mocking)
• fix failing test: test_util.test_merge_esmvaltool_datasets

[Daafip]

Not too sure how to complete renaming from ERA-5 to ESMValTool compatible variable names.
Do you have any insight on how to do that @BSchilperoort?

Edit: for now just supplied a rename dictionary as in discussion

[BSchilperoort]

Hi David, nice work! I left some comments for possible improvements to the code. Feel free to ask a review again when it's ready!

[BSchilperoort]

Also, it would be nice if you could add tests for this forcing. (without downloading anything). If you're unsure on how to do that we can discuss it at some point

[RolfHut]

regarding conventions: from a didactic point of view in tutorial notebooks I suggest to always use:

import ewatercycle.forcing

camels_forcing = ewatercycle.forcing.sources['LumpedCaravanForcing'].retrieve(start_time = experiment_start_date,
                                                          end_time = experiment_end_date,
                                                          directory = forcing_path / "Camels",
                                                          basin_id = f"camels_0{HRU_id}",
                                                          variables = ('streamflow','potential_evaporation_sum'),
                                                          )

Novice users quickly loose sight of which function belongs to which library when using from ewatercycle.forcing import sources. This might load a bit more than you would ideally want, but communicates more clearly. In code 'under the hood' I'm more than fine with the shorter version.

[Daafip]

Made the suggested changes Rolf, Example Notebook can be found here:
https://gist.github.com/Daafip/ac1b030eb5563a76f4d02175f2716fd7

[Daafip]

Still facing three issues, maybe you could help with that Bart?:

• test_utipl.test_merge_esmvaltool_datasets fails: . If i run the test locally in my IDE I don't have this problem but on the git tests I Do...?

FAILED tests/src/test_util.py::test_merge_esmvaltool_datasets - AssertionError: assert 'height' in {'cell_methods': 'day_of_year: year: mean', 'long_name': 'Near-Surface Air Temperature', 'standard_name': 'air_temperature', 'units': 'K'}

• MyPy throws an error I dont understand in the SonarCloud analysis:

mypy.....................................................................Failed
-hook id: mypy
-exit code: 1

src/ewatercycle/_forcings/caravan.py:10: error: Library stubs not installed for "requests"  [import]
src/ewatercycle/_forcings/caravan.py:10: note: Hint: "python3 -m pip install types-requests"
src/ewatercycle/_forcings/caravan.py:10: note: (or run "mypy --install-types" to install all missing stub packages)
src/ewatercycle/_forcings/caravan.py:10: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports

• Not sure how to make tests not download

[BSchilperoort]

test_utipl.test_merge_esmvaltool_datasets fails: . If i run the test locally in my IDE I don't have this problem but on the git tests I Do...?

That's odd. The "height" attribute should be in there. Perhaps it's an environment/versioning issue, and the environment is newly created on Github Actions for the test. So you don't see the error as you don't have all the latest versions of packages.

MyPy throws an error I dont understand in the SonarCloud analysis:

Probably also due to a version change. You can add the types-request package to the setup.cfg file below types-PyYAML. That should fix it.

Not sure how to make tests not download

You would have to mock/patch some of the code. For examples see

ewatercycle/tests/src/test_container.py

Line 177 in b4ea453

def mock_bmi_client_apptainer():

You'd mock this line in your code ds = xr.open_dataset(f"{OPENDAP_URL}{dataset}.nc"), with mock.patch("xarray.Dataset') as ....
You can then set the return value to return a dataset you load from a small testing file. And assert that the mocked function was called with the correct arguments.

[BSchilperoort]

Hi David, we found the test issue and fixed it in #410 . Once that's merged to main that should resolve the problem you encounter here.

On second thought I do think that it would be better to use the generate method to generate the forcing. You can add extra kwargs (i.e. basin_id, and just leave the shape one unused).

From the converage output in the CI I see this;

src/ewatercycle/_forcings/caravan.py                   85     28     32      3    60%

Generally we want 80% coverage on the code (sonarcloud analysis). I think if you add tests for the extract_basin_shapefile function that you might be there already.

After these things I think this PR is ready to merge! 👍

[Daafip]

I think if you add tests for the extract_basin_shapefile

Done! That Bumps it up to 83%
src/ewatercycle/_forcings/caravan.py 84 11 32 3 83%

On second thought I do think that it would be better to use the generate method to generate the forcing. You can add extra kwargs (i.e. basin_id, and just leave the shape one unused).

Refctored this now, so all ready!

Pytest fails on the other test, sonar cloud is stuck on the token but should both be fine.

Thanks for all the input & feedback @BSchilperoort @sverhoeven!

[Daafip]

apart from the technical comments, a more hydrological point: we do have to make sure in the documentation that people are aware that the Caravan dataset is not "just" the Camels grouped together, but rather it is new (ERA) data derived for the shape files in the Camels datasets. See https://egusphere.copernicus.org/preprints/2024/egusphere-2024-864/ for a ongoing discussion in the hydrological community about this.

To get back to you on this @RolfHut, In the data set attributes under history it does mention the data source. This is now correctly copied over to the new datas set. If the user looks at the (meta)data, they should see its ERA5

[BSchilperoort]

Can you add where the user can lookup the basin ID?

[Daafip]

See this is where geopandas would be nice :p, but will add a function which lets the user explore the dataset(ids).

[BSchilperoort]

I was hoping more for something like a webpage listing the basin_id's together with river/basin names, country, etc. I guess that doesn't exist?

[Daafip]

Nope, but what i do now is list the datasets in a seperate function. That way the user can get the dataset as netcdf and a list of basin ID's

[Daafip]

For advanced users this will do, for more novice users choosing a single catchment: likely they will have to download the combinedshapefile, load this into GIS and pick one

[Daafip]

I was hoping more for something like a webpage listing the basin_id's together with river/basin names, country, etc. I guess that doesn't exist?

would be pretty easy to make an interactive folium/leaflet map out of this actually! Similar to this

[Daafip]

Lets continue this discussion in issues: #398

[BSchilperoort]

Nice work! Sorry that it was blocked a bit by the failing test+sonarcloud problems, but now it's ready to merge (after that 1 last comment above, and pre-commit haha) 😄

Edit: InputError isn't a buildin exception. You can pick a different appropriate one. This is why mypy fails.