-
Notifications
You must be signed in to change notification settings - Fork 4
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
GTC-2898 Modify resolution metadata fields #585
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## develop #585 +/- ##
===========================================
+ Coverage 81.12% 81.18% +0.06%
===========================================
Files 128 129 +1
Lines 5821 5841 +20
===========================================
+ Hits 4722 4742 +20
Misses 1099 1099
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good (except for my one comment), though I don't know the full details on how alembic migrations are supposed to look.
response.json()["data"]["metadata"]["resolution"] | ||
== version_metadata["resolution"] | ||
response.json()["data"]["metadata"]["spatial_resolution"] | ||
== version_metadata["spatial_resolution"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you fix this case in the deprecated test, maybe also fix the case at line 212 (change resolution to "spatial_resolution", and also probably "title" to "spatial_resolution".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks ok but have a question on migration: since you're dropping the resolution
column, do you have a script that'd migrate the existing resolution
data to the new column?
@danscales pointed out alter_column
that I missed in first review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
I was assuming that his use of |
Pull request checklist
Please check if your PR fulfills the following requirements:
Pull request type
Please check the type of change your PR introduces:
What is the current behavior?
Issue Number: GTC-2898
Currently one metadata field for resolution,
resolution
, representing spatial resolution in square meters.What is the new behavior?
spatial_resolution
metadata field (int, STAC-compliant) for spatial resolution in square metersresolution_description
metadata field (string) for front-end display purposes, such as 10 hectares, 10 degrees, etc.)Does this introduce a breaking change?
Other information