Irregular rebinning function and Analysis Subpackage#726

Open

DanRyanIrish wants to merge 8 commits into

sunpy:main from

DanRyanIrish:irregular_rebin

Open

Irregular rebinning function and Analysis Subpackage #726
DanRyanIrish wants to merge 8 commits into
sunpy:main from
DanRyanIrish:irregular_rebin

Conversation

@DanRyanIrish

@DanRyanIrish DanRyanIrish commented Jun 12, 2024 •

edited

Loading

Copy link

Copy Markdown

Member

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

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

To Consider

Change name of rebin_by_edges to rebin_by_corners

DanRyanIrish added 2 commits

June 11, 2024 14:24

@DanRyanIrish


 Create analysis subpackage and create rebin function.

b5dac0b

This just moves the rebin code, but the rebin method remains in NDCube.

@DanRyanIrish


 Add rebin_by_edges function to analysis subpackage.

c38adfe

@DanRyanIrish DanRyanIrish requested review from Cadair and samaloney

June 12, 2024 12:11

@DanRyanIrish DanRyanIrish added this to the 2.4.0 milestone

Jun 12, 2024

DanRyanIrish added 3 commits

June 12, 2024 14:13

@DanRyanIrish


 Remove rogue .swp file.

e34c461

@DanRyanIrish


 Add changelogs for PR sunpy#726 .

de3f3fc

@DanRyanIrish


 Fix codestyle.

c2923aa

@DanRyanIrish DanRyanIrish force-pushed the irregular_rebin branch from 133d184 to c2923aa Compare

June 12, 2024 12:26

@settwi

settwi commented Jun 13, 2024

Copy link

Copy Markdown

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.

samaloney

samaloney reviewed

Jun 13, 2024

View reviewed changes

@samaloney samaloney left a comment

Copy link

Copy Markdown

Member

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

changelog/726.feature.1.rst Outdated

changelog/726.feature.3.rst Outdated

ndcube/analysis/rebin.py

@DanRyanIrish @samaloney


 Minor fixes suggested in code review

19563ee

Co-authored-by: Shane Maloney <maloneys@tcd.ie>

@DanRyanIrish

DanRyanIrish commented Jun 14, 2024

Copy link

Copy Markdown

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?

@DanRyanIrish


 Add rebin_by_edges to __all__

b7e5a0e

DanRyanIrish

DanRyanIrish commented

Jun 25, 2024

View reviewed changes

ndcube/analysis/rebin.py Outdated

from ndcube.utils.exceptions import warn_user

from ndcube.wcs.wrappers import ResampledLowLevelWCS

__all__ = ["rebin"]

@DanRyanIrish DanRyanIrish Jun 25, 2024

Copy link

Copy Markdown

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 DanRyanIrish mentioned this pull request

Jun 26, 2024

Provide a shorthand for "one bin along this axis" in .rebin #737

Closed

@DanRyanIrish

DanRyanIrish commented Jul 1, 2024

Copy link

Copy Markdown

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.

Cadair

Cadair reviewed

Jul 2, 2024

View reviewed changes

ndcube/ndcube.py

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):

@Cadair Cadair Jul 2, 2024 •

edited

Loading

Copy link

Copy Markdown

Member

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?

@DanRyanIrish DanRyanIrish Dec 18, 2024

Copy link

Copy Markdown

Member Author

Choose a reason for hiding this comment

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

@Cadair can you elaborate on this? From the example in that link it looks like it's serving the wrong direction to the one we need...

Cadair

Cadair reviewed

Jul 2, 2024

View reviewed changes

ndcube/analysis/rebin.py

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

@Cadair Cadair Jul 2, 2024

Copy link

Copy Markdown

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.

@DanRyanIrish DanRyanIrish Jul 3, 2024

Copy link

Copy Markdown

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.

@github-actions

github-actions Bot commented Dec 1, 2024

Copy link

Copy Markdown

Contributor

Hello 👋, Thanks for your contribution to ndcube!
I have marked this pull request as stale because there hasn't had any activity in five months. If you are still working on this, or if it's waiting on a maintainer to look at it then please let us know and we will keep it open. Please add a comment with: @sunpy/ndcube-developers to get someone's attention.
If nobody comments on this pull request for another month, it will be closed.

@github-actions github-actions Bot added the Stale label

Dec 1, 2024

@DanRyanIrish


 Merge branch 'main' of https://github.com/sunpy/ndcube into irregular...

d3627ab

..._rebin

@github-actions

github-actions Bot commented Feb 14, 2025

Copy link

Copy Markdown

Contributor

Hello again 👋, We want to thank you again for your contribution to ndcube!
This pull request has had no activity since my last reminder, so I am going to close it. If at any time you want to come back to this please feel free to reopen it! If you want to discuss this, please add a comment with: @sunpy/ndcube-developers and someone will get back to you soon.

@github-actions github-actions Bot closed this

Feb 14, 2025

@samaloney

samaloney commented Feb 19, 2026 •

edited

Loading

Copy link

Copy Markdown

Member

PING for sunkit-spex and potentially radiospectra

@samaloney samaloney reopened this

Feb 19, 2026

@github-actions github-actions Bot removed the Stale label

Feb 20, 2026

Labels

None yet

4 participants

@DanRyanIrish @settwi @samaloney @Cadair

Uh oh!

Conversation

@DanRyanIrish DanRyanIrish commented Jun 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Description

Uh oh!

settwi commented Jun 13, 2024

Uh oh!

@samaloney samaloney left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DanRyanIrish commented Jun 14, 2024

Uh oh!

@DanRyanIrish DanRyanIrish Jun 25, 2024

Choose a reason for hiding this comment

Uh oh!

DanRyanIrish commented Jul 1, 2024

Uh oh!

@Cadair Cadair Jul 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@DanRyanIrish DanRyanIrish Dec 18, 2024

Choose a reason for hiding this comment

Uh oh!

@Cadair Cadair Jul 2, 2024

Choose a reason for hiding this comment

Uh oh!

@DanRyanIrish DanRyanIrish Jul 3, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Dec 1, 2024

Uh oh!

github-actions Bot commented Feb 14, 2025

Uh oh!

samaloney commented Feb 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

@DanRyanIrish DanRyanIrish commented Jun 12, 2024 •

edited

Loading

@Cadair Cadair Jul 2, 2024 •

edited

Loading

samaloney commented Feb 19, 2026 •

edited

Loading