Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add a convex hull masking function #237
Add a convex hull masking function #237
Changes from 1 commit
556d710
6283778
86d496e
a382d88
8a92291
d1ebe64
8bc30fe
f08b120
178b467
c938047
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Does delaunay still use qhull library which uses 32 floats (or something else weird) at some point of it's processing pipeline. This forces one to use "normalized" coordinates, otherwise points are truncated to a "grid"
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.
That's a good question. From the docs, I would say "yes": https://docs.scipy.org/doc/scipy/reference/generated/scipy.spatial.Delaunay.html#scipy.spatial.Delaunay but they don't say anything about that.
What do you mean by "normalized"? Scale them to [0, 1]? If you have any pointers that would be great :)
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 found this page but there is nothing on that: http://www.qhull.org/html/qh-impre.htm
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.
By the way, thanks for finding these things Ari. Always things I would never have thought to look for 👍
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.
Normalized --> small enough
I once did stuff with 1000 m x 1000 m area (points in random position) with real coordinates (northern sweden, some coordinate system with large values) and had this problem. It took sometime to figure out why results were funny.
Good thing is that can be done only for the data going in the Delaunay and then use the original data for rest of the workflow.
Maybe some simple script with large offset from origo could tell if this is still a problem or not.
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.
Thanks for the pointers! I'll try to add a test for that to see if it's something we need to worry about.
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.
https://gist.github.com/ahartikainen/bf2c0eb2ed493040ff490e0c97435ade
Not so sure if this can also fail for convex hull, probably it can
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.
Ah, that's severe actually. It probably will since the process I'm using is checking if points fall on any simplex. I'm add this and see if the gallery example changes much.
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 added the normalization code and support for projecting the coordinates prior to the masking so we can get really large coordinates. This is the gallery example without normalization:
And this is after normalization:
So no difference in this case. I'll leave the normalization code in there since it doesn't seem to hurt the results and might prevent headaches in the future.
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.
By choosing the
northing
andeasting
based on the order ofdims
could be a source of errors if the dimensions are not in the expected order.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 thought of this after #229 but we're getting dims from the
DataArray
so ii should be fine. The only thing that might cause problems is if the dims are different for the different vars. I could add a check for that and error if that's the case.This is why adding these things back to xarray is hard. I don't think I could generalize this to arbitrary
Dataset
.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.
The problem may appear if the user is using a different order on the
dims
of theDataArray
. I've seen that after applying somexarray
functions, sometimes the order ofdims
are changed (sorry, don't remember exactly which ones, but found this issue that may be related).I don't have a final solution for this, just wanted to raise the discussion.
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.
Right, I hadn't thought of that. It might be worth mentioning in the docs that we're assuming this order of dimensions then.