Fixes a bug when the source and target run in opposite directions - #407
Fixes a bug when the source and target run in opposite directions#407HGWright wants to merge 3 commits into
Conversation
Sorry, something went wrong.
| def test_wrong_rising_target(self): | ||
| r = self.interpolate([2, 1], [1, 2]) | ||
| assert_array_equal(r, [1, np.inf]) | ||
| assert_array_equal(r, [0.0, 1.0]) |
There was a problem hiding this comment.
We know that there are extra steps taking place in this test. But it is not indicative of the simple interpolation of this data, as shown by the example below.
import numpy as np import stratify z_targ = np.array([2, 1]) z_source = np.array([1, 2]) data_source = z_source.copy() out = stratify.interpolate(z_targ, z_source, data_source) print(out)
[2. 1.]
Sorry, something went wrong.
There was a problem hiding this comment.
Sure. But it doesn't seem appropriate to try and preserve this test, just with a new Known Good Output, right? If I understand correctly, we no longer expect it to be possible to have a 'wrong' rising target - it should always work?
Sorry, something went wrong.
trexfeathers
left a comment
There was a problem hiding this comment.
Thanks @HGWright, this seems a pragmatic solution. Some rough edges to deal with though
Sorry, something went wrong.
| tgt_indexer = [slice(None)] * z_target.ndim | ||
| tgt_indexer[tgt_axis] = slice(0, 2) | ||
| tgt_first_two = z_target[tuple(tgt_indexer)].ravel()[:2] |
There was a problem hiding this comment.
Is there a reason that this is different logic to the indexing of z_src?
python-stratify/src/stratify/_vinterp.pyx
Lines 634 to 637 in 9396b41
If it's just a matter of taste/style, I would be keen to see the same logic for both. Could even be put into a convenience function.
Sorry, something went wrong.
| def test_wrong_rising_target(self): | ||
| r = self.interpolate([2, 1], [1, 2]) | ||
| assert_array_equal(r, [1, np.inf]) | ||
| assert_array_equal(r, [0.0, 1.0]) |
There was a problem hiding this comment.
Sure. But it doesn't seem appropriate to try and preserve this test, just with a new Known Good Output, right? If I understand correctly, we no longer expect it to be possible to have a 'wrong' rising target - it should always work?
Sorry, something went wrong.
| def test_non_monotonic_coordinate_interp(self): | ||
| result = self.interpolate([15, 5, 15.0], [10.0, 20, 0, 20]) | ||
| assert_array_equal(result, [1, 2, 3]) | ||
| assert_array_equal(result, [1.0, 1.0, 2.0]) |
There was a problem hiding this comment.
The dtype stuff definitely needs to be sorted out - that's gonna break some user assumptions. Especially since it only affects stuff that goes through the flipping process (am I right?), so the dtype will depend on the direction of the inputs, that doesn't seem right.
Sorry, something went wrong.
Reviewers
Requested changes must be addressed to merge this pull request.
🚀 Pull Request
Description
Closes #50
This is a replacement PR for #343 that uses a simpler approach to achieve the same goal. In the cases that rising = None we use the first 2 points of the source to determine rising, we then also use the same method for the target. Comparing rising (for the source) to tgt_rising if they do not match, we flip the source and source data on the axis of interpolation.