Skip to content
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.

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

Support generating individual pyramid levels #84

Merged
merged 15 commits into from
Mar 19, 2024
Merged

Support generating individual pyramid levels #84

merged 15 commits into from
Mar 19, 2024

Conversation

maxrjones
Copy link
Contributor

@maxrjones maxrjones commented Jan 11, 2024

This separates out the reprojection of individual levels into a separate function, as a pre-req for #74 and pangeo-forge/pangeo-forge-recipes#669.

Copy link

codspeed-hq bot commented Jan 12, 2024

CodSpeed Performance Report

Merging #84 will not alter performance

⚠️ No base runs were found

Falling back to comparing split-levels (db69f1a) with main (b6c5f4f)

Summary

✅ 12 untouched benchmarks

@maxrjones maxrjones changed the title WIP: Support single level re-projection WIP: Support generating individual pyramid levels Jan 18, 2024
@maxrjones maxrjones self-assigned this Mar 7, 2024
@maxrjones maxrjones changed the title WIP: Support generating individual pyramid levels Support generating individual pyramid levels Mar 18, 2024
@norlandrhagen norlandrhagen self-requested a review March 18, 2024 22:56
@@ -1,5 +1,6 @@
# flake8: noqa

from .core import pyramid_coarsen, pyramid_reproject
from .coarsen import pyramid_coarsen
from .reproject import pyramid_reproject
Copy link
Contributor

Choose a reason for hiding this comment

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

Splitting these up is a nice touch.

----------
ds : xarray.Dataset
The dataset to coarsen.
factors : list[int]
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if it's worth it in this PR, but I wonder if it would be useful to have some checking on these input factors in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed! since this is focused on the pyramid_reproject, I think it would work better as a separate PR

def level_reproject(
ds: xr.Dataset,
*,
projection: typing.Literal['web-mercator', 'equidistant-cylindrical'] = 'web-mercator',
Copy link
Contributor

Choose a reason for hiding this comment

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

This type hinting is neat! I like that it saves a check on the projection being on of a set of values.

for k, da in ds.items():
if clear_attrs:
da.attrs.clear()
if len(da.shape) == 4:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be ≥ 4? or should there be another case if we shave ds.shape == 5?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it would break for 5+ dim datasets. Perhaps we should add a check and raise an informative error as a separate PR

ndpyramid/reproject.py Outdated Show resolved Hide resolved
Co-authored-by: Raphael Hagen <norlandrhagen@gmail.com>
@maxrjones maxrjones added the benchmark Run benchmarks on PR label Mar 18, 2024
@maxrjones maxrjones merged commit 90dc72b into main Mar 19, 2024
7 checks passed
@maxrjones maxrjones deleted the split-levels branch March 19, 2024 00:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Run benchmarks on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants