Skip to content
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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

DanRyanIrish
Copy link
Member

@DanRyanIrish DanRyanIrish commented Jun 12, 2024

PR Description

Following the structure outline by #725, this PR creates an analysis subpackage and a rebin module. Included in the rebin module is the implementation of the rebin method, which now simply calls the rebin function.

However, crucially, it also includes a rebin_by_edges function, which allows users to rebin an NDCube 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 an NDCube method. However, if this limitation is overcome in the future, an NDCube.rebin_by_edges method can be implemented as a wrapper around this function.

To Do

  • Change name of rebin_by_edges to rebin_by_corners
  • Restore function signature to NDCube.rebin. Explicitly list out inputs, rather than using *args and `**kwargs``.
  • Currently, wcs and extra coords are lumped together into combined wcs. Separate these so output still has same coords in WCS and extra_coords
  • Incorporate feature request in Provide a shorthand for "one bin along this axis" in .rebin #737
  • Write tests for rebin_by_corners
  • Fix doc test

@DanRyanIrish DanRyanIrish added this to the 2.4.0 milestone Jun 12, 2024
@settwi
Copy link

settwi commented Jun 13, 2024

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.

Copy link
Contributor

@samaloney samaloney left a 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.

ndcube/analysis/rebin.py Outdated Show resolved Hide resolved
changelog/726.feature.1.rst Outdated Show resolved Hide resolved
changelog/726.feature.3.rst Outdated Show resolved Hide resolved
ndcube/analysis/rebin.py Show resolved Hide resolved
@DanRyanIrish
Copy link
Member Author

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 NDCube.reproject_to, assuming you generate the new WCS separately, which, I grant, can be a more cumbersome API. I am right though? Is there a difference between rebin_interpolated and reproject_to other than a more restrictive and hence more friendly API?

from ndcube.utils.exceptions import warn_user
from ndcube.wcs.wrappers import ResampledLowLevelWCS

__all__ = ["rebin"]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
__all__ = ["rebin"]
__all__ = ["rebin", "rebin_by_edges"]

@DanRyanIrish
Copy link
Member Author

We should consider whether rebin_by_edges should be rebin_by_corners to be consistent with the axis_world_coords(corners=True) API.

@@ -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):
Copy link
Member

@Cadair Cadair Jul 2, 2024

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?

Comment on lines +361 to +368
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
Copy link
Member

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.

Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants