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

Cif 197 core resolution option for layer downloads #68

Merged
merged 37 commits into from
Sep 6, 2024

Conversation

kcartier-wri
Copy link
Contributor

Is this PR related to a JIRA ticket?

https://gfw.atlassian.net/browse/CIF-197

What changes were proposed in this PR?

Allow control of spatial resolution of raster layers.

How was this patch tested?

Locally on MS WSL

Did this PR include necessary documentation updates?

  • No, this PR does not affect any public API so no need to change the docs.

@kcartier-wri
Copy link
Contributor Author

I implemented the test_layer_parameters test in a manner that will simplify addition of other layer classes in the future. This solution follows the goal of making CIF as self-maintainable as possible by data scientists.

@kcartier-wri kcartier-wri marked this pull request as ready for review August 30, 2024 01:58
@kcartier-wri kcartier-wri marked this pull request as draft August 30, 2024 01:59
@kcartier-wri
Copy link
Contributor Author

Change summary:

  1. Added spatial_resolution parameter to scalable classes with default value
  2. Implemented test_layer_pararameters to test changes to spatial_resolution
  3. Upgraded requirements files for xee version to resolve an issue where image_collections had been returned in inconsistent projections, not necessarily the requested crs. (This took some trial and error commits to resolve. Next time, I’ll use GitHub Codespaces.)

@kcartier-wri kcartier-wri marked this pull request as ready for review September 4, 2024 17:11
Copy link
Member

@jterry64 jterry64 left a comment

Choose a reason for hiding this comment

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

Minor suggestion for additional tests but otherwise looks good!

tests/test_layer_parameters.py Outdated Show resolved Hide resolved
tests/test_layer_parameters.py Outdated Show resolved Hide resolved
@kcartier-wri kcartier-wri removed the request for review from chrowe September 4, 2024 18:58
@kcartier-wri
Copy link
Contributor Author

@jterry64 - In particular, could you carefully review the spatial_resolution implementation in HighLandSurfaceTemperature?

@kcartier-wri kcartier-wri requested review from jterry64 and chrowe and removed request for jterry64 and chrowe September 5, 2024 00:01
tests/test_layer_parameters.py Outdated Show resolved Hide resolved
@kcartier-wri kcartier-wri merged commit 5cdd466 into main Sep 6, 2024
1 check passed
@kcartier-wri kcartier-wri deleted the CIF-197-CORE-Resolution-option-for-layer-downloads branch September 6, 2024 18:32
Copy link
Contributor

@chrowe chrowe left a comment

Choose a reason for hiding this comment

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

I know this is merged but had a couple questions...

Attributes:
start_date: starting date for data retrieval
end_date: ending date for data retrieval
spatial_resolution: raster resolution in meters (see https://github.com/stac-extensions/raster)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason you added (see https://github.com/stac-extensions/raster) to all of theses and not just the ones that use a stac catalog?

@@ -16,5 +16,6 @@ geemap==0.32.0
pip==23.3.1
boto3==1.34.124
scikit-learn==1.5.1
scikit-image==0.24.0
Copy link
Contributor

Choose a reason for hiding this comment

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

What are we using scikit-image for?

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.

3 participants