Implement Superpixel on NDCube #450
Conversation
|
Hello @DanRyanIrish! Thanks for updating this PR. There are currently no PEP 8 issues detected in this Pull Request. Cheers! Comment last updated at 2021-11-18 14:23:40 UTC |
DanRyanIrish
changed the title
The factors were incorrectly introduced because symmetry around pixel 0 was overlooked. The version of the code in this commit is equivalent to the original version but it slightly tidier and more efficient.
This method interpolate coords given a new array grid and returns a new ExtraCoords object with the interpolated coords. Also use this method in NDCube.superpixel.
|
@Cadair: The oldestdeps tests are failing due to a DeprecationWarning being raised. @nabobalis have tried to make this ignored but against all insistence from us, the warning is still raised? Any ideas how to fix this? If not, the only options may be to merge with this failing or bump the oldest deps. |
|
@Cadair : Thanks to some help from Nabil, this PR is ready for another review. I plan to add a couple more tests to check correct errors are raised and bump up the coverage. Apart from that though, this PR could be merged. |
DanRyanIrish
force-pushed
the
superpixel
branch
from
e5fe2ff
to
cc4b8f7
Compare
There was a problem hiding this comment.
Looking great!
I have left a bunch of mostly minor comments. I will admit I haven't gone through and understood the depths of all the code, but it looks good.
It seems the coverage 🤖 isn't very happy, don't suppose you have the time to cover some of the edge / error cases with tests?
haha just noticed your comment about doing that.
| if isinstance(new_data, ARRAY_MASK_MAP[np.ndarray]): | ||
| new_data = new_data.data |
There was a problem hiding this comment.
This still weirds me out. It feels like we have special cased dask and numpy here, where numpy does this but dask dosen't. What if cupy adds a masked array where we need to do this. It all makes me uncomfortable.
There was a problem hiding this comment.
You're right. But since numpy and dask masked arrays have different APIs, the alternative I see is to force everything into numpy masked arrays which is a worse idea.
| # Execute rebin. | ||
| cube = ndcube_2d_dask | ||
| bin_shape = (4, 2) | ||
| output = cube.rebin(bin_shape, propagate_uncertainties=True) |
There was a problem hiding this comment.
hmm, I would have to confirm that, but probably.
Co-authored-by: Stuart Mumford <stuart@cadair.com>
DanRyanIrish
force-pushed
the
superpixel
branch
from
9ec66ec
to
d59b208
Compare
|
@Cadair I think this is ready for a final review. |
| @property | ||
| @abc.abstractproperty | ||
| def is_empty(self): | ||
| """Return True if no extra coords present, else return False.""" |
There was a problem hiding this comment.
| """Return True if no extra coords present, else return False.""" | |
| """ | |
| Return True if no extra coords present, else return False. | |
| """ |
Description
This PR implements a
superpixelmethod onNDCube(now renamedrebin) that downsamples the grid by a factor that is an integer fraction of the cube's length in the relevant dimension. It enables the user to provide the function that dictates how the value in the superpixel is derived from its constituent pixels. To support this, aninterpolatemethod has been introduced toExtraCoords.For reviewing purposes, the core pieces of this PR are
NDCube.rebinandExtraCoords.interpolate.This PR depends on #449
To Do
superpixeltorebinmaxandminversions. If multiple pixels with max/min value, preserve the larger uncertaintyNDArithmeticMixinkwargpropagate_uncertaintieswhich has a different behaviour.use_masked_valuestooperation_ignores_mask