-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
alternate fix for issue #997 #1477
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
Merged
Merged
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
6065697
delaunay linear_interpolate_grid: enable grid of height/width 1
49215c2
delaunay: test for 1d and 0d grids (fails upstream) re: issue #997
764571b
Merge branch 'iss997_test' into iss997_altfix
9fb8c3c
test_delaunay: avoid numbers in 1d test, use np.allclose
AmitAronovitch 7d3d2f4
Merge branch 'iss997_test'
AmitAronovitch 964a854
_delaunay.cpp: support [x0:x1:1j] grids even if x1!=x0
AmitAronovitch File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next
Next commit
delaunay linear_interpolate_grid: enable grid of height/width 1
fixes #997
- Loading branch information
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
Perhaps I am missing something here, but if x1==x0, then wouldn't the operation return zero anyway (unless xsteps==1)? If all we are doing is preventing division by zero errors, then wouldn't we rather want to test for xsteps==1?
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 agree with @WeatherGod here, xsteps==1 (and similar for y) is better 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.
Also, as a bit of sanity check, is it ever possible for x/ysteps to be zero?
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.
Argument validation for the grid specification arguments could be done on the python side. Thats basically in delaunay.interpolate.LinearInterpolator.getitem (but we should grep for other possible references).
Currently no validation is done. If we want to add them, we should think about the possible semantics of various cases.
To @WeatherGod (2): 0 for x/ysteps would just return an empty grid. If you put a negative value, it would fail before reaching the loop, because numpy raises an exception if you try to allocate an array with negative dimenson.
To @WeatherGod (1), @ianthomas23 : To be exact, I was checking for 0/0 (which is nan) rather than the general */0 (which would be "inf" unless the * is also 0).
The case with xsteps==1 and x0!=x1 may have valid use-cases, but it is not clear where the single point should be. Setting dx=0 in this case is equivalent to putting it on x0. We could, for example place it at 0.5*(x0+x1), or at x1. The stable code would fill the array with inf, which may be considered as some kind of error-indication.
On the other hand, I had actually used the case (x0==x1, xsteps>1) before, and from my point of view I was merely "extending its range of validity" to xsteps==0, without affecting the case described above.
However: I tend to think that having the "*/0" case produce x0 is actually better than the current "inf", and I do agree that it makes the code somewhat more readable.
I'll accept your suggestion - just making sure you understand the implications.
btw: the other fix (#998) has the advantage that the edge cases are naturally resolved, and you do not have to think about all these cases (I guess it would also produce x0 rather than inf, but I did not check that).