-
Notifications
You must be signed in to change notification settings - Fork 2
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
Nearest neighbor interpolator #12
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.
Looks good! Had a couple of comments about testing saving the interpolator and one minor comment on sampling for the tests.
earth2grid/_regrid.py
Outdated
return output | ||
|
||
|
||
class S2LinearBarycentricInterpolator(torch.nn.Module): |
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.
Would be good to add a test that saves a checkpoint of an initialized instance of this and loads that back in. Would be very useful to avoid repeated calls to spatial.KDTree
especially in ETL jobs where a regridding script might be invoked multiple times on different inputs.
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'll make an issue for this. I think it may require some thought and potentially backwards breaking changes to apply this consistently across the suite of re-gridders.
I'm not sure the most elegant way to allow ser/deser of "buffer-only" modules like this. Do you know of an example for this e.g. with normalization.
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 think we don't necessarily need a proper serialization/deserialization pipeline here right? It might be sufficient to create a classmethod that takes in a state dict and returns an initialized instance from that?
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 refactored this so that the S2NearestNeighborInterpolator
now returns a serializeable Regridder
object. Let me know what you think.
tests/test_regrid.py
Outdated
n = 10000 | ||
torch.manual_seed(0) | ||
lon = torch.rand(n) * 360 | ||
lat = torch.rand(n) * 180 - 90 |
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.
lat = torch.rand(n) * 180 - 90 | |
lat = torch.asin(2*torch.rand(n)-1) * 180. / np.pi |
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 intent here?
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.
This isn't a super important change, but thought it would be good to remove the polar bias when sampling on the sphere in these tests.
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.
Changes look good and the unification of the two KDTree implementations is nice too.
Not for this PR, but might be nice to have the Regridder
object be TorchScript compatible so you could serialize a regridder object that way.
No description provided.