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

[WIP - DRAFT] Ndpyramid #669

Closed
wants to merge 85 commits into from

Conversation

norlandrhagen
Copy link
Contributor

Draft PR to explore generating pyramids via pangeo-forge-recipes. Uses ndpyarmid.

Copy link
Contributor

@maxrjones maxrjones left a comment

Choose a reason for hiding this comment

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

Sharing some comments from a code review with @norlandrhagen.

Another note is that we should eventually look at memory usage on a local and flink runner

pangeo_forge_recipes/transforms.py Outdated Show resolved Hide resolved
Default is None.
:param pyramid_kwargs: Dict containing any kwargs that should be passed to ndpyramid.
Default is None.

Copy link
Contributor

Choose a reason for hiding this comment

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

Eventually we should add tests for the two currently supported projections. Since this is only relevant for one of the methods, pyramid_reproject I think it should go in pyramid_kwargs rather than a separate kwarg.

Default is None.
:param pyramid_kwargs: Dict containing any kwargs that should be passed to ndpyramid.
Default is None.

Copy link
Contributor

Choose a reason for hiding this comment

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

Eventually it could be nice it include pyramid_method to use coarsen, regrid, or reproject rather than just reproject

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call! I updated the transform to include those methods and raise a NotImplimentedError for now if it's not reproject.

pangeo_forge_recipes/transforms.py Outdated Show resolved Hide resolved
Comment on lines +718 to +721
chunks = {"x": self.pixels_per_tile, "y": self.pixels_per_tile}
if self.other_chunks is not None:
chunks |= self.other_chunks

Copy link
Contributor

Choose a reason for hiding this comment

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

This is blocked by #520


ds = xr.Dataset(attrs=attrs)

ds.to_zarr(store=f"{self.target_root.root_path}/{self.store_name}") # noqa
Copy link
Contributor

Choose a reason for hiding this comment

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

Could compute=False be good to include here?

tests/test_end_to_end.py Outdated Show resolved Hide resolved
tests/test_end_to_end.py Show resolved Hide resolved
@norlandrhagen
Copy link
Contributor Author

Closing this PR in favor or https://github.com/carbonplan/pangeo-forge-ndpyramid.

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.

None yet

2 participants