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

Gb/nc chunks #124

Merged
merged 13 commits into from
Nov 28, 2022
Merged

Gb/nc chunks #124

merged 13 commits into from
Nov 28, 2022

Conversation

grantbuster
Copy link
Member

No description provided.

Copy link
Collaborator

@bnb32 bnb32 left a 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.


class DataHandlerNC(DataHandler):
"""Data Handler for NETCDF data"""

CHUNKS = {'XTIME': 100, 'XLAT': 100, 'XLON': 100}
Copy link
Collaborator

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.

Copy link
Member Author

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?

Copy link
Member Author

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?

Copy link
Collaborator

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?

Copy link
Collaborator

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".

Copy link
Member Author

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:
Copy link
Collaborator

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?

Copy link
Member Author

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.

Copy link
Collaborator

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?

Copy link
Member Author

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

Copy link
Collaborator

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

Copy link
Member Author

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah good point

@grantbuster grantbuster merged commit 1cceaf4 into main Nov 28, 2022
@grantbuster grantbuster deleted the gb/nc_chunks branch November 28, 2022 21:05
github-actions bot pushed a commit that referenced this pull request Nov 28, 2022
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.

None yet

2 participants