-
Notifications
You must be signed in to change notification settings - Fork 8
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
Gb/nc chunks #124
Gb/nc chunks #124
Conversation
…inear correction factors
…ch the target level (e.g. at surface w level=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.
Looks good. Just a couple questions/comments.
sup3r/preprocessing/data_handling.py
Outdated
|
||
class DataHandlerNC(DataHandler): | ||
"""Data Handler for NETCDF data""" | ||
|
||
CHUNKS = {'XTIME': 100, 'XLAT': 100, 'XLON': 100} |
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.
Maybe we should add south_north and west_east here since those are common dimension names.
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.
Yep agreed, do they have those names exactly? south_north
and west_east
? Any others we should add?
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.
Also do you want to change the chunk sizes at all?
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.
Yeah those are the exact names. I'm currently using 120x120 for fwp_shape so maybe we could put the default somewhere above that. 150x150?
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.
We might also want to add "bottom_top" for the pressure levels and "Time".
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.
Can you actually just add a commit and make any mods you think are appropriate?
# slope will be NaN. This is most common if you have multiple pressure | ||
# levels at zero height at the surface in the case that the data didnt | ||
# provide underground data. | ||
for level in levels: |
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.
Is this perturbing the levels so they dont overlap?
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.
Correct, for example if the data has a floor geopotential height of 0m, multiple vertical levels are at 0m, and you're interpolating to 0m, the interpolation will throw an error. Adding a bit of random noise to any levels that are exactly at the target interpolation level is an easy fix.
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.
Yeah that makes sense. Is there any weirdness that might come from this noise?
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.
Not really, adding 1e-5 to the x values in a lin interp shouldn't really change anything
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.
Yeah that was my impression as well
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 should also add that this is incredibly rare - you should basically never get vertical levels exactly at the value you're interpolating to. This really only happens when we fix a floor at surface.
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.
Yeah good point
No description provided.