Combine cubes from NDCubeSequence using reproject #436

adwaitbhope · 2021-07-04T14:57:50Z

Description

Fixes #430.

I'm currently only assuming one sequence axis, that part needs to be handled better.
I'm also exposing an axis parameter for stacking data which will affect the mapping passed on to the CompoundLowLevelWCS.

pep8speaks · 2021-07-04T14:57:53Z

Hello @adwaitbhope! Thanks for updating this PR.

In the file ndcube/ndcube_sequence.py:

Line 123:25: W503 line break before binary operator

Comment last updated at 2022-02-28 12:42:32 UTC

adwaitbhope · 2021-07-04T14:58:15Z

Will add tests soon.

Cadair

This looks like a great start. I have only looked at the main stack function at the moment.

By biggest high-level comment would be that we probably want to be a little smarter about how we generate the new combined WCS. I have left a couple of comments inline with specific suggestions. It also might make sense to have a function which generates that new WCS which we can then swap out if/when we add support for other ways of adding WCS dimensions.

ndcube/ndcube_sequence.py

Cadair · 2021-07-05T13:46:10Z

ndcube/ndcube_sequence.py

+        axis: `int`, optional
+            The axis along which the data is to be stacked.


This is a little confusing. What this is is the array axis we are stacking along.

I do wonder if we even want this to be a configurable parameter. If we are being purists about it then the slowest varying coordinate should go last (which it is a safe assumption is the sequence dimension). I would propose maybe just dropping this parameter all together. I wonder if @DanRyanIrish or @wtbarnes have differing opinions on that though.

For now, I've dropped the axis parameter to make things a little simpler while computing the pixel to world axis mapping.

ndcube/ndcube_sequence.py

DanRyanIrish

I think this is looking good.

ndcube/ndcube_sequence.py

Cadair

This looks like it's heading in the right direction, I would like to test it out myself tomorrow.

ndcube/ndcube_sequence.py

DanRyanIrish

I think this looks good. I just have a request regarding extracting the physical_types. See in-line comments.

ndcube/ndcube_sequence.py

DanRyanIrish · 2021-08-02T14:06:42Z

ndcube/ndcube_sequence.py

+        for axis_name, axis_coords in self.sequence_axis_coords.items():
+            if isinstance(axis_coords[0], (u.Quantity, numbers.Number)):
+                table_coords.append(QuantityTableCoordinate(u.Quantity(axis_coords),
+                                                            names=axis_name))


In each case, try to pull the physical_type of each coord from the first cube in the sequence and use it to set the physical_type of each TableCoordinate.

@adwaitbhope Any progress on this?

I have now. I'm extracting the physical type from global_coords. But since we're iterating through .items() of the sequence_axis_coords dictionary, their order doesn't seem to be the same at every run.

I'm sorting the wcs.world_axis_physical_types in the tests due to this, is that alright?

DanRyanIrish · 2021-08-02T14:16:13Z

This also needs a changelog.

Cadair

I think this looks good. I wonder if @wtbarnes has had a chance to test it 🤔

changelog/436.feature.rst

ndcube/ndcube_sequence.py

DanRyanIrish · 2021-08-23T08:25:32Z

@wtbarnes have you or are you planning to test this out?

wtbarnes · 2021-08-23T13:33:12Z

@wtbarnes have you or are you planning to test this out?

Sorry I probably won't have time in the next day or two.

hayesla

This is great - just needs to be rebased with main.

My only concern is (I'm wearing a map hat here) is that this can take a long time for 2D images. This is of course down to reproject_to here, but even with like 16 AIA images at 256x256 took 7 mins on my local machine. In this way, I think a user warning or something could be added to warn of this?

but other than taking a long time, its working as expected!

ndcube/ndcube_sequence.py

hayesla · 2021-11-26T15:10:39Z

ndcube/ndcube_sequence.py

+        # Reproject all cubes to a common WCS
+        target_wcs = self[common_wcs_index].wcs
+        shape_out = self[common_wcs_index].data.shape
+        reprojected_cubes = [cube.reproject_to(target_wcs, shape_out=shape_out,


maybe a warning or something should be flagged here if this reproject is going to take a long time. Imagine a user has a sequence of a hundred 4Kx4K AIA images and then tried to run this, it will take a looonng time

Co-authored-by: DanRyanIrish <ryand5@tcd.ie>

Co-authored-by: Stuart Mumford <stuart@cadair.com>

Co-authored-by: Laura Hayes <hayesla@tcd.ie>

github-actions · 2022-07-29T05:13:28Z

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 · 2022-08-28T05:14:41Z

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.

Cadair reviewed Jul 5, 2021

View reviewed changes

DanRyanIrish reviewed Jul 12, 2021

View reviewed changes

ndcube/ndcube_sequence.py Outdated Show resolved Hide resolved

Cadair reviewed Jul 12, 2021

View reviewed changes

ndcube/ndcube_sequence.py Outdated Show resolved Hide resolved

ndcube/ndcube_sequence.py Outdated Show resolved Hide resolved

Cadair reviewed Jul 20, 2021

View reviewed changes

ndcube/ndcube_sequence.py Outdated Show resolved Hide resolved

Cadair reviewed Jul 20, 2021

View reviewed changes

ndcube/ndcube_sequence.py Show resolved Hide resolved

Cadair reviewed Jul 20, 2021

View reviewed changes

ndcube/ndcube_sequence.py Outdated Show resolved Hide resolved

Cadair reviewed Jul 20, 2021

View reviewed changes

ndcube/ndcube_sequence.py Outdated Show resolved Hide resolved

DanRyanIrish reviewed Jul 21, 2021

View reviewed changes

ndcube/ndcube_sequence.py Show resolved Hide resolved

adwaitbhope force-pushed the reproject-ndcubesequence branch from faa3b0a to f640ebe Compare July 21, 2021 11:26

DanRyanIrish reviewed Aug 2, 2021

View reviewed changes

DanRyanIrish added this to the 2.1 milestone Aug 2, 2021

adwaitbhope mentioned this pull request Aug 5, 2021

Reproject NDCubeSequence Documentation #441

Closed

Cadair reviewed Aug 6, 2021

View reviewed changes

changelog/436.feature.rst Outdated Show resolved Hide resolved

ndcube/ndcube_sequence.py Outdated Show resolved Hide resolved

ndcube/ndcube_sequence.py Outdated Show resolved Hide resolved

adwaitbhope force-pushed the reproject-ndcubesequence branch from fe8025f to dc69a1c Compare August 22, 2021 18:58

hayesla reviewed Nov 26, 2021

View reviewed changes

adwaitbhope and others added 10 commits February 28, 2022 17:33

Initial implementation of using reproject in NDCubeSequence

eaf6a1a

Improve computation of CompoundWCS

b1750ae

Improve mapping for Compound WCS

22d4de0

Remove axis parameter

43c4f0d

Add test for reprojecting NDCubeSequence

32690bf

Refactor sequence axes wcs

f8f0d1c

Add sequence axis names to wcs

8970f71

Add docstring

97f75ea

Minor fix

8533f65

Update ndcube/ndcube_sequence.py

264e7f6

Co-authored-by: DanRyanIrish <ryand5@tcd.ie>

adwaitbhope and others added 18 commits February 28, 2022 17:36

Update ndcube/ndcube_sequence.py

a6beb6b

Co-authored-by: Stuart Mumford <stuart@cadair.com>

Minor fix

9b0ff01

Throw exception for invalid global_coords

9bb06d0

Add test for invalid global_coords

5946d33

Minor changes

bea7c4f

Update ndcube/ndcube_sequence.py

603a270

Co-authored-by: Stuart Mumford <stuart@cadair.com>

Update ndcube/ndcube_sequence.py

92fd854

Co-authored-by: Stuart Mumford <stuart@cadair.com>

Improve get sequence axes wcs

060ec4c

Add tests

00524b1

Add check for cube shape

4b0e350

Add test for varying cube shape

247fd87

Add physical_type to TableCoordinate

905bb58

Add changelog

cf4a365

Update changelog/436.feature.rst

6786314

Co-authored-by: Stuart Mumford <stuart@cadair.com>

Update extracting physical type from global_coords

88500ea

Correct docstring

fa6595f

Fix failing tests

4cb9c51

Fix codestyle tests

8a95305

adwaitbhope force-pushed the reproject-ndcubesequence branch from dc69a1c to 8a95305 Compare February 28, 2022 12:41

Update ndcube/ndcube_sequence.py

8cc4451

Co-authored-by: Laura Hayes <hayesla@tcd.ie>

github-actions bot added the stale label Jul 29, 2022

github-actions bot closed this Aug 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Combine cubes from NDCubeSequence using reproject #436

Combine cubes from NDCubeSequence using reproject #436

adwaitbhope commented Jul 4, 2021

pep8speaks commented Jul 4, 2021 •

edited

adwaitbhope commented Jul 4, 2021

Cadair left a comment

Cadair Jul 5, 2021

adwaitbhope Jul 9, 2021

DanRyanIrish left a comment

Cadair left a comment

DanRyanIrish left a comment

DanRyanIrish Aug 2, 2021

DanRyanIrish Aug 4, 2021

adwaitbhope Aug 5, 2021 •

edited

DanRyanIrish commented Aug 2, 2021

Cadair left a comment

DanRyanIrish commented Aug 23, 2021

wtbarnes commented Aug 23, 2021

hayesla left a comment

hayesla Nov 26, 2021

github-actions bot commented Jul 29, 2022

github-actions bot commented Aug 28, 2022

		axis: `int`, optional
		The axis along which the data is to be stacked.

Combine cubes from NDCubeSequence using reproject #436

Combine cubes from NDCubeSequence using reproject #436

Conversation

adwaitbhope commented Jul 4, 2021

Description

pep8speaks commented Jul 4, 2021 • edited

Comment last updated at 2022-02-28 12:42:32 UTC

adwaitbhope commented Jul 4, 2021

Cadair left a comment

Choose a reason for hiding this comment

Cadair Jul 5, 2021

Choose a reason for hiding this comment

adwaitbhope Jul 9, 2021

Choose a reason for hiding this comment

DanRyanIrish left a comment

Choose a reason for hiding this comment

Cadair left a comment

Choose a reason for hiding this comment

DanRyanIrish left a comment

Choose a reason for hiding this comment

DanRyanIrish Aug 2, 2021

Choose a reason for hiding this comment

DanRyanIrish Aug 4, 2021

Choose a reason for hiding this comment

adwaitbhope Aug 5, 2021 • edited

Choose a reason for hiding this comment

DanRyanIrish commented Aug 2, 2021

Cadair left a comment

Choose a reason for hiding this comment

DanRyanIrish commented Aug 23, 2021

wtbarnes commented Aug 23, 2021

hayesla left a comment

Choose a reason for hiding this comment

hayesla Nov 26, 2021

Choose a reason for hiding this comment

github-actions bot commented Jul 29, 2022

github-actions bot commented Aug 28, 2022

pep8speaks commented Jul 4, 2021 •

edited

adwaitbhope Aug 5, 2021 •

edited