-
Notifications
You must be signed in to change notification settings - Fork 1
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
DAS-1934: Most basic implementation to allow umm-grid definitions #14
DAS-1934: Most basic implementation to allow umm-grid definitions #14
Conversation
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 work!
The request changes is really only to update the release note extraction script. But I would be interested to hear your thoughts on the whole coupling of x and y message parameters (that might mean some more in-the-weeds code changes). If you think it doesn't matter too much, I'll run through the tests quickly, because everything otherwise looks great.
### 2024-04-05 | ||
# Changelog | ||
|
||
## [v1.1.0] - 2024-08-29 |
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.
I like this update to the change log format, but you'll need to make a quick change to the script that extracts information from here to make the release notes.
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.
I never remember this. Thanks ed0ff0e
@@ -122,14 +124,13 @@ def get_parameters_from_message( | |||
if parameters['interpolation'] in [None, '', 'None']: | |||
parameters['interpolation'] = INTERPOLATION_DEFAULT | |||
|
|||
# ERROR 5: -tr and -ts options cannot be used at the same time. | |||
# when a user requests both a resolution and dimensions, then ensure the | |||
# extents are consistent. | |||
if (parameters['xres'] is not None or parameters['yres'] is not None) and ( | |||
parameters['height'] is not None or parameters['width'] is not None | |||
): |
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.
What is the behaviour when only one of the dimensions (x or y) has both the resolution and the height or width defined?
I think there's maybe something to think about here, here and here, because I think now it is possible to have a self consistent grid that has (for example) scaleExtent
, scaleSize
and then only one of height
and/or width
.
I think, with these changes, it technically makes no difference - what would happen is that the code would calculate the missing value (say width
) and then also calculate height
. And because the grid is self-consistent, the value of height
would come out the same, but it feels a bit clunky to recalculate something we already have.
I guess the conclusion is: the code isn't doing anything wrong, or getting a bad value, but there's a coupling between the x and y grid parameters that means the code doesn't quite make as much sense in the case outlined. Feel free to tell me this doesn't matter too much, because it ends up all the same, but it feels like maybe it's better to decouple x and y to make things easier to grok.
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.
I'm not 100% sure I understand you fully.
From what I see this function is called at the beginning of reproject() inside adaper's process_item
What is the behaviour when only one of the dimensions (x or y) has both the resolution and the height or width defined?
Then if the user only gave a single value height
or width
, it would wipe out whichever one was input here and recalculate both.
Same thing would happen if a single resolutions was input with valid dimensions and extents.
It feels a bit clunky to recalculate something we already have.
True, but I'm a big meh about it.
there's a coupling between the x and y grid parameters that means the code doesn't quite make as much sense in the case outlined.
I feel like you're trying to get at something with this, but I don't know what it is. The only difference I can see is that we don't automatically fail when you over describe your input paramters. Adding an single extra dimension that matches the input is kinda dump but it doesn't break anything, and if you provide both we don't do any recomputations (beyond the ones we did to verify the grid)
So I need more information but leaning towards a big shrug.
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.
Looking more closely here, I guess that decoupling x and y is perhaps more tricky than I'd first thought. Perhaps, I'm backing down a little bit here on my initial thoughts: yes it feels clunky/unintuitive to couple x and y the way they are now, but maybe there isn't a clean way to separate them in some cases.
Maybe I'm beginning to also say 🤷
[v1.1.0]:(https://github.com/nasa/harmony-swath-projector/releases/tag/1.0.1) | ||
[v1.0.1]:(https://github.com/nasa/harmony-swath-projector/releases/tag/1.0.1) | ||
[v1.0.0]:(https://github.com/nasa/harmony-swath-projector/releases/tag/1.0.0) |
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.
@owenlittlejohns this is actually different from how I did it on some of the other changelogs, but I like this better. this is from common changelog and takes you to the release instead of the diff of the changes.
Also updates CHANGELOG.md to add links for previous versions.
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.
I was unable to build the Docker image locally to test these changes. Looks like there's an issue with building the wheel for pyresample
:
25.04 ERROR: Failed building wheel for pyresample
25.04 ERROR: ERROR: Failed to build installable wheels for some pyproject.toml based projects (pyresample)
It's weird - it's happening in bin/build-image
but not just in an environment on my Mac.
I wonder if this is a M1 issue. Or maybe I didn't try to build the images? I don't have my intel machine so will have to wait until I get back to see. But I am having problems with my environment for nsidc-regression tests on this M1 also, submitting the harmony requests is failing. |
Ok I just looked at my test instructions and I did not have issues building on my mac before. Yup, I can't build on this M1. We probably need to figure out how to deal with ARM64. |
This may not be the best way to do this anymore, the link to the docs is gone, but this did let me get it running on M1. Here's the link https://docs.docker.com/desktop/troubleshoot/known-issues/ PUlled into this PR |
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.
Thanks for tweaking the release note script - that ran as expected for me locally.
I ran the test request locally, and it completed nicely. Here are some pretty pictures of Europe from least to most psychodelic:
With regards to the coupling of x and y dimensions. I think that given the coupling in the underlying calculations from the swath, and that a self consistent grid will just recalculate an identical number for the use-case I mentioned (3/3 grid parameters in one dimension and 2/3 grid parameters in the other), then probably I was getting myself into a twist over something that isn't a big deal.
Nice work!
Description
Changes behavior when the input parameters consist of both a resolutions (
xres
andyres
) and dimensions (height
andwidth
)Jira Issue ID
DAS-1934
Local Test Steps
FIRST: Verify test request fails.
You can do this by running a test request in UAT
https://harmony.uat.earthdata.nasa.gov/C1233860183-EEDTEST/ogc-api-coverages/1.0.0/collections/all/coverage/rangeset?forceAsync=true&interpolation=near&granuleId=G1233860486-EEDTEST&grid=GEOS1x1test
Or to be super careful. you can pull the current 1.0.1 image locally and run it in your Harmony-In-A-Box :
Start up Harmony-In-A-Box with this image.
Run this test request and see that the request fails:
http://localhost:3000/C1233860183-EEDTEST/ogc-api-coverages/1.0.0/collections/all/coverage/rangeset?forceAsync=true&interpolation=near&granuleId=G1233860486-EEDTEST&grid=GEOS1x1test
You should see the same error as in UAT.
Now verify the fix worked:
Check out this branch, build and run the docker and test images.
Ensure the tests pass.
Ran 82 tests in 34.151s OK
Reload Harmony-In-A-Box to pick up this latest image.
Try the command again and verify it does not fail with an error.
http://localhost:3000/C1233860183-EEDTEST/ogc-api-coverages/1.0.0/collections/all/coverage/rangeset?forceAsync=true&interpolation=near&granuleId=G1233860486-EEDTEST&grid=GEOS1x1test
PR Acceptance Checklist
CHANGELOG.md
updated to include high level summary of PR changes.docker/service_version.txt
updated if publishing a release.