Skip to content

478 caravan forcing downloads and unzips shapefiles for all caravan with each basin leads to errors fixes #478 #479

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.

Sign up for GitHub

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

Open
RolfHut wants to merge 5 commits into
base: main
Choose a base branch
from
Open

478 caravan forcing downloads and unzips shapefiles for all caravan with each basin leads to errors fixes #478 #479

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
86bc9a9
fixed download problem, still problem with file handling
RolfHut May 20, 2025
2e8fdc0
fixed it :-).
RolfHut May 20, 2025
a22b6d8
removed white space
RolfHut May 20, 2025
f93f00f
fixed typos based on lint feedback
RolfHut May 20, 2025
ca884ee
blech minor details
RolfHut May 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 15 additions & 6 deletions src/ewatercycle/_forcings/caravan.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@
end_time: str,
directory: str,
variables: tuple[str, ...] = (),
shape: str | Path | None = None,
shape_in: str | Path | None = None,
**kwargs,
) -> "CaravanForcing":
"""Retrieve caravan for a model.
Expand All @@ -164,7 +164,8 @@
directory: Directory in which forcing should be written.
variables: Variables which are needed for model,
if not specified will default to all.
shape: (Optional) Path to a shape file.
shape_in: (Optional) Path to a shape file of the basin, or the combined.shp
file of all basins.
If none is specified, will be downloaded automatically.
kwargs: Additional keyword arguments.
basin_id: The ID of the desired basin. Data sets can be explored using
Expand All @@ -188,8 +189,19 @@
ds_basin = ds.sel(basin_id=basin_id.encode())
ds_basin_time = crop_ds(ds_basin, start_time, end_time)

if shape is None:
if shape_in is None:
shape = get_shapefiles(Path(directory), basin_id)
elif Path(shape_in).name == "combined.shp":
Copy link
Copy Markdown
Member

@BSchilperoort BSchilperoort May 21, 2025

Choose a reason for hiding this comment

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

This would not be the most robust way of checking for an already existing combined shapefile. It's also a bit difficult to document.

Maybe it would be better to;

  • Not download automatically anymore if shape=None, although this breaks existing code using this feature
  • Add a keyword argument (like the existing basin_id one) for downloading the shapes, download_shape which defaults to false.
# get download_shape from kwargs, default to false;
download_shape = kwargs.get("download_shape", False)
  • Add another kwarg combined_shapefile that you can use to point to the combined shapefile with all basins
combined_shape = kwargs.get("combined_shape")
if combined_shape is not None:
    combined_shape = Path(combined_shape)
    if not combined_shape.exists():
        msg = "Could not find combined shapefile."
        raise FileNotFoundError(msg)

These kwargs can be properly documented in the docstring, and validating the user input is a bit easier this way.

shape = Path(directory) / f"{basin_id}.shp"
extract_basin_shapefile(basin_id, Path(shape_in), shape)

Check warning on line 196 in src/ewatercycle/_forcings/caravan.py

View check run for this annotation

Codecov / codecov/patch

src/ewatercycle/_forcings/caravan.py#L195-L196 

Added lines #L195 - L196 were not covered by tests
elif Path(shape).name != f"{basin_id}.shp":
msg = (

Check warning on line 198 in src/ewatercycle/_forcings/caravan.py

View check run for this annotation

Codecov / codecov/patch

src/ewatercycle/_forcings/caravan.py#L198 

Added line #L198 was not covered by tests
"shape must either point to a shapefile of the basin ID"
"Or to the combined.shp file that contains all basins."
)
raise ValueError(msg)

Check warning on line 202 in src/ewatercycle/_forcings/caravan.py

View check run for this annotation

Codecov / codecov/patch

src/ewatercycle/_forcings/caravan.py#L202 

Added line #L202 was not covered by tests
else:
shape = shape_in

Check warning on line 204 in src/ewatercycle/_forcings/caravan.py

View check run for this annotation

Codecov / codecov/patch

src/ewatercycle/_forcings/caravan.py#L204 

Added line #L204 was not covered by tests

if len(variables) == 0:
variables = ds_basin_time.data_vars.keys() # type: ignore[assignment]
Expand Down Expand Up @@ -299,9 +311,6 @@
# kind of clunky but it works: select filtered polygon
if i == basin_index:
geom = feat.geometry
if geom.type != "Polygon":
msg = "Only polygons are supported"
raise ValueError(msg)

# Add the signed area of the polygon and a timestamp
# to the feature properties map.
Expand Down
Loading