-
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
Changes from 7 commits
bb7ebd8
d072bca
addf305
8150470
44589d8
f9890f0
ed0ff0e
e2b0453
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
1.0.1 | ||
1.1.0 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,10 +7,12 @@ | |
from typing import Dict | ||
|
||
from harmony.message import Message | ||
from harmony.message_utility import has_self_consistent_grid | ||
from pyproj import Proj | ||
from varinfo import VarInfoFromNetCDF4 | ||
|
||
from swath_projector import nc_merge | ||
from swath_projector.exceptions import InvalidTargetGrid | ||
from swath_projector.interpolation import resample_all_variables | ||
|
||
RADIUS_EARTH_METRES = ( | ||
|
@@ -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 commentThe 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) I think, with these changes, it technically makes no difference - what would happen is that the code would calculate the missing value (say 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 commentThe 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
Then if the user only gave a single value
True, but I'm a big meh about it.
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 commentThe 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 🤷 |
||
raise Exception( | ||
'"scaleSize", "width" or/and "height" cannot ' | ||
'be used at the same time in the message.' | ||
) | ||
if not has_self_consistent_grid(message): | ||
raise InvalidTargetGrid() | ||
|
||
if not os.path.isfile(parameters['input_file']): | ||
raise Exception('Input file does not exist') | ||
|
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