-
-
Notifications
You must be signed in to change notification settings - Fork 48
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
Irregular rebinning function and Analysis Subpackage #726
base: main
Are you sure you want to change the base?
Conversation
This just moves the rebin code, but the rebin method remains in NDCube.
133d184
to
c2923aa
Compare
Hey Dan this looks really good. I especially like the option to propagate uncertainties (and presumably other metadata, too?). It will save us headaches in other applications. Occasionally it is nice to do some interpolation of bins, too, like when binning a spectral response down from fine photon bins to coarser count bins, or to "directly" compare data from different instruments with different energy bins. For histograms you want to conserve the "flux" in each bin and the simplest way to do it is just a proportional interpolation. For photon count or flux data I reckon this approach makes sense. I made a gist a while back to do an interpolating rebinning which has proved useful a few times. Might be nice to add a similar functionality to ndcube. Maybe "rebin_interpolated" or something like that. "Normal" rebinning is just a subset of the interpolating version, but it might make sense to specialize the implementations to save on computation time. |
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.
This is GREAT! I still need to actually try it out but just figured get these down now.
Co-authored-by: Shane Maloney <[email protected]>
Hi @settwi. Thanks for the comments. I definitely agree with the use-cases you pointed out. From a technical perspective, I think they are already served by |
from ndcube.utils.exceptions import warn_user | ||
from ndcube.wcs.wrappers import ResampledLowLevelWCS | ||
|
||
__all__ = ["rebin"] |
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.
__all__ = ["rebin"] | |
__all__ = ["rebin", "rebin_by_edges"] |
We should consider whether |
@@ -996,8 +996,7 @@ def to(self, new_unit, **kwargs): | |||
new_unit = u.Unit(new_unit) | |||
return self * (self.unit.to(new_unit, **kwargs) * new_unit / self.unit) | |||
|
|||
def rebin(self, bin_shape, operation=np.mean, operation_ignores_mask=False, handle_mask=np.all, | |||
propagate_uncertainties=False, new_unit=None, **kwargs): | |||
def rebin(self, *args, **kwargs): |
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.
We should wrap this to maintain the signature. Perhaps we could use this: https://docs.python.org/3/library/functools.html#functools.wraps to avoid the copy?
ec = ExtraCoords() | ||
for name, axis, coord, physical_type in zip( | ||
new_coord_names, new_coord_pix_idxs, new_coord_values, new_coord_physical_types | ||
): | ||
if len(axis) != 1: | ||
raise NotImplementedError("Rebinning multi-dimensional coordinates not supported.") | ||
ec.add(name, axis, coord, physical_types=physical_type) | ||
new_wcs = ec.wcs |
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.
Using extra coords as a way to do this is a bit "weird" using table coordinate directly might be more applicable.
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.
Would this require extra code to parse whether it should be a QuantityTableCoordinate
, TimeTableCoordinate
, etc.? If so, ExtraCoords
already does this for us.
PR Description
Following the structure outline by #725, this PR creates an
analysis
subpackage and arebin
module. Included in therebin
module is the implementation of therebin
method, which now simply calls therebin
function.However, crucially, it also includes a
rebin_by_edges
function, which allows users to rebin anNDCube
which irregular bin sizes along an axis. This is a very important functionality for certain analyses, e.g. solar X-ray imaging spectroscopy.Due to technical limitations,
ndcube
can currently only support this if the WCS axes are independent, and so is not within the scope of anNDCube
method. However, if this limitation is overcome in the future, anNDCube.rebin_by_edges
method can be implemented as a wrapper around this function.To Do
rebin_by_edges
torebin_by_corners
NDCube.rebin
. Explicitly list out inputs, rather than using*args
and `**kwargs``.rebin_by_corners