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

Implement ind_from_latlon using a BallTree #59

Merged
merged 17 commits into from
Jun 13, 2023
Merged

Conversation

leuty
Copy link
Contributor

@leuty leuty commented Mar 23, 2023

I tried using ind_from_latlon on with the new ICON-1 domain of MCH and it was kinda slow (20 Min). I implemented a new version using a BallTree.

I also added functionality to return the n closest points. That is super useful if one want to compare against instruments and avoid double penalties by slight location offsets.

@leuty leuty changed the title Implement ind_from_latlon using a cKDTree Implement ind_from_latlon using a K-D Tree Mar 23, 2023
@leuty
Copy link
Contributor Author

leuty commented Mar 23, 2023

Note, an version for spherical earth can be found here:

https://github.com/Unidata/python-workshop/blob/fall-2016/notebooks/netcdf-by-coordinates.ipynb

Copy link
Collaborator

@victoria-cherkas victoria-cherkas left a comment

Choose a reason for hiding this comment

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

Breaking changes (ind_from_latlon is used in scripts in icon-vis):

  • verbose argument is removed
  • returns a List[int] rather than int.

Also I think its nice to have an example usage section in the docstring.

@AnnikaLau what do you think about the above? Perhaps its nicer to return a List[int] and then in our icon-vis scripts we can just select ind[0].
And I personally thing we should keep the verbose option. But let me know what you think.

@AnnikaLau
Copy link
Collaborator

Thanks for the update/improvement! I'm still going through it but one thing I'm not sure of yet, would ind[0] be the closest of the closest points?

@leuty
Copy link
Contributor Author

leuty commented May 15, 2023

@AnnikaLau
I think the distinction is made by either passing k=n or k=[n] to cKDTree.query, see here.

Would it help if I re-implemented the verbose option for ind_from_latlon ? Then you can test.

@AnnikaLau
Copy link
Collaborator

@AnnikaLau I think the distinction is made by either passing k=n or k=[n] to cKDTree.query, see here.

Would it help if I re-implemented the verbose option for ind_from_latlon ? Then you can test.

Yes I think the verbose option is useful for the case k=1, which we use for the icon-vis examples. So that the users are aware of that the location of the nearest point might be quite different. So it would be great if you could re-implement it for that case.

@leuty
Copy link
Contributor Author

leuty commented May 16, 2023

@AnnikaLau
So that the users are aware of that the location of the nearest point might be quite different.

I dont understand. Does it yield a different location for your case?

@AnnikaLau
Copy link
Collaborator

@AnnikaLau So that the users are aware of that the location of the nearest point might be quite different.

I dont understand. Does it yield a different location for your case?

It takes the nearest point to the coordinates given. So in the case the grid cells are big, this can be quite a bit off.

iconarray/core/utilities.py Outdated Show resolved Hide resolved
iconarray/core/utilities.py Outdated Show resolved Hide resolved
iconarray/core/utilities.py Outdated Show resolved Hide resolved
leuty and others added 2 commits May 16, 2023 15:06
@AnnikaLau
Copy link
Collaborator

I created a corresponding merge request for icon-vis: C2SM/icon-vis#39

Comment on lines 90 to 91
>>> lats = np.rad2deg(ds.clat.values[:])
>>> lons = np.rad2deg(ds.clon.values[:])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
>>> lats = np.rad2deg(ds.clat.values[:])
>>> lons = np.rad2deg(ds.clon.values[:])
>>> lats = xr.DataArray(np.rad2deg(data.clat), coords=data.clat.coords, dims=data.clat.dims)
>>> lons = xr.DataArray(np.rad2deg(data.clon), coords=data.clon.coords, dims=data.clon.dims)

Copy link
Collaborator

Choose a reason for hiding this comment

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

The old version doesn't work anymore with the changes to the script.

Copy link
Contributor Author

@leuty leuty May 31, 2023

Choose a reason for hiding this comment

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

I dont understand why exactly it stopped, but fine with me.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I get errors when executing the next line otherwise

Copy link
Contributor Author

@leuty leuty Jun 8, 2023

Choose a reason for hiding this comment

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

is this still present? Cant reproduce

@leuty leuty changed the title Implement ind_from_latlon using a K-D Tree Implement ind_from_latlon using a BTree May 31, 2023
@leuty leuty changed the title Implement ind_from_latlon using a BTree Implement ind_from_latlon using a BallTree May 31, 2023
@leuty
Copy link
Contributor Author

leuty commented May 31, 2023

@AnnikaLau
I realized that my approach did not work with global domains. I now use another Tree Data Structure and Haversines to compute the distances. The downside is that it adds another dependency: skikit-learn.

@AnnikaLau
Copy link
Collaborator

Radian and degree are now mixed up for input and output. I would only give degree as input and output as it is much more intuitive.

@leuty leuty requested a review from AnnikaLau June 8, 2023 16:42
@AnnikaLau
Copy link
Collaborator

Apart from one last thing, which should be fixed, I think it looks good and should be merged together with C2SM/icon-vis#39.
However, the tests are currently failing on icon-vis, so we should wait until this is fixed before we can merge it...

Co-authored-by: Annika <[email protected]>
Copy link
Collaborator

@AnnikaLau AnnikaLau left a comment

Choose a reason for hiding this comment

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

Waiting for approval of C2SM/icon-vis#39

@AnnikaLau AnnikaLau merged commit 6aca696 into C2SM:main Jun 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants