Skip to content

Bbox forcings def - #235

Open
aldotapia wants to merge 7 commits into
CIROH-UA:mainfrom
aldotapia:bbox_forcings_def
Open

Bbox forcings def#235
aldotapia wants to merge 7 commits into
CIROH-UA:mainfrom
aldotapia:bbox_forcings_def

Conversation

@aldotapia

Copy link
Copy Markdown
Contributor

New feature: bbox definition for raw forcings download

Context: evaluation of storm-displacement scenarios using NGIAB. Since storm displacement is part of the analysis, one way to generate new scenarios is to displace one or all forcings (here using NWM or AORC data sources) as a way to displace a storm spatially via affine transformation plus any interpolation method to compute the new pixel values. Since the bounding box of the raw forcing equals the polygon bounding box, using current library functions could lead to some gaps in the borders.

The approach overcomes this limitation by taking a larger bbox like:

new_storm_diagram

Schematically, the change looks like:

bbox

New features:

The CLI incorporates two new arguments:

--bbox: min, ymin, xmax and ymax coordinates of the custom boundary box to download. By default, the boundary box CRS is WGS84 geographical (EPSG:4326).

--bbox_crs: EPSG code to define the CRS of the bounding box

Important note: if the custom bbox is smaller than the vector's bbox, a ValueError will raise (key function here:bbox_contains)

How it was implemented

By setting the bounds for clip_dataset_to_bounds function.

Testing

Just add the custom bbox and the CRS to the command line:

python -m ngiab_data_cli.forcing_cli \
  -i /path/to/geopackage.gpkg \
  -o /path/to/forcings/forcings.nc \
  --start 2018-05-20 --end 2018-05-32 \
  --source aorc \
  --bbox 1345760.7 1675911.0 1945760.7 2275911.0 --bbox_crs EPSG:5070

This PR sets the number of workers based on either an env variable configuration fix or via command line
Just applied black formatter to PEP 8 compliance
Raw forcings can be clipped by a custom bbox (with CRS definition if needed). If the bbox is smaller than the catchment's bbox, it will force to use catchment's bbox to avoid issues with the zonal statistics process
saving the raw forcing was failing while using Dask multiprocessor

@quinnylee quinnylee left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Successfully ran the new bounding box feature with uv run cli -i cat-1555522 -sfr --start 2022-01-01 --end 2022-01-02 --source aorc --bbox -100 40 -90 35

Interesting feature idea! Overall I think it can get included, but the PR's scope is too large, since it also includes some dask changes.

Also, please update the README with documentation on the new CLI flags. Tests would also be nice

Comment thread modules/data_processing/dataset_utils.py
Comment thread modules/data_processing/dataset_utils.py
try:
tol_x = abs(float(cached_data.x.values[1] - cached_data.x.values[0]))
tol_y = abs(float(cached_data.y.values[1] - cached_data.y.values[0]))
except (IndexError, ValueError):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

When would these exceptions show up? When the cached data is a 1x1 grid?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Exactly, if the cached area was small enough. This is unlikely to happen due to the NWM and AORC pixel size, but there are divides small enough to trigger it (for example, cat-108690 with an area of 0.001799 km^2)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ok cool! A clarifying comment there would be great!

Comment thread modules/data_processing/dataset_utils.py
Addition of `--bbox`, `--bbox_crs` and `--dask-workers` to documentation
@aldotapia

Copy link
Copy Markdown
Contributor Author

Documentation added. I will let you know after I add the tests

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.

2 participants