Datacube core #1669

rcooke-ast · 2023-09-11T13:02:18Z

This looks like a large PR, but it's a relatively simple refactoring to make the CoAdd3D look a lot more like CoAdd2D. Here are the main changes:

IFU reduction is replaced throughout the code by SlicerIFU, with a view to possibly support FiberIFU as another reduction mode in the (distant) future.
pypeit.core.datacube.py has been refactored to core routines and code that uses PypeIt-specific objects.
Unused routines from pypeit.core.datacube.py have been moved to deprecated

I should emphasise that there's virtually no new code here. It's just a reorganising of the code that makes datacubes so it's easier for others to read, improve, and it mostly just required writing a lot more docstrings... joy!

I'm pretty sure that nothing is broken, but I'll run the tests just to be sure...

codecov-commenter · 2023-09-11T13:13:59Z

Codecov Report

Merging #1669 (3ad7d3c) into develop (b23ec14) will increase coverage by 0.03%.
The diff coverage is 10.94%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@             Coverage Diff             @@
##           develop    #1669      +/-   ##
===========================================
+ Coverage    41.05%   41.09%   +0.03%     
===========================================
  Files          189      190       +1     
  Lines        43568    43621      +53     
===========================================
+ Hits         17888    17926      +38     
- Misses       25680    25695      +15

Files	Coverage Δ
pypeit/core/datacube.py	`8.27% <ø> (+2.69%)`	⬆️
pypeit/core/meta.py	`86.95% <ø> (ø)`
pypeit/display/display.py	`8.00% <ø> (ø)`
pypeit/find_objects.py	`45.95% <100.00%> (ø)`
pypeit/flatfield.py	`53.48% <ø> (ø)`
pypeit/pypeit.py	`77.12% <100.00%> (ø)`
pypeit/scripts/show_2dspec.py	`11.73% <0.00%> (ø)`
pypeit/core/flat.py	`50.30% <0.00%> (ø)`
pypeit/par/pypeitpar.py	`96.03% <60.00%> (-0.05%)`	⬇️
pypeit/slittrace.py	`30.14% <50.00%> (-0.05%)`	⬇️
... and 9 more

rcooke-ast

Addressed PR comments

doc/coadd3d.rst

pypeit/core/extract.py

rcooke-ast · 2023-10-04T15:21:42Z

pypeit/coadd3d.py

+        """
+        Utility function to provide the world coordinate system of the datacube
+        """
+        return wcs.WCS(self.head0)


I can see the benefit of not computing ivar repeatedly, but I worry that we'll then be carrying around self._ivar and self.ivar, which seems a little messy to me. I also suspect it's rare that the ivar will ever be used... and it's also pretty quick to generate self.wcs. Also, is there a point in having the @property and function in this case? Couldn't we just have self.ivar = utils.inverse(self.sig**2) when a DataCube gets loaded?

Overall, I'm tempted to leave things the way they are, but it might be that I'm not appreciating the full benefit of this, so if you think this really ought to be done, then should it be done in the from_file() routine?

pypeit/coadd3d.py

doc/coadd3d.rst

pypeit/spectrographs/gemini_gnirs.py

pypeit/spectrographs/keck_kcwi.py

pypeit/tests/test_ref_index.py

pypeit/coadd3d.py

jhennawi · 2023-10-08T10:32:32Z

pypeit/coadd3d.py

+            msgs.info("Adopting a square pixel spatial scale of {0:f} arcsec".format(3600.0 * self._dspat))
+        # If the user has not specified the spectral sampling, then set it now to the largest value
+        if self._dwv is None:
+            self._dwv = np.max(self._specscale)


Can you return these values instead of assigning to self. For a small number of return values that makes the control flow more transparent.

Good catch - I agree 👍 Actually, I decided to move this entire section of code to core.

jhennawi · 2023-10-08T10:35:44Z

pypeit/coadd3d.py

+            sensfunc = self.flux_spline(senswave)
+
+        # Generate a datacube
+        outfile = datacube.get_output_filename("", self.cubepar['output_filename'], True, -1)


My advice is to create the this outfile name int he init method or before coadd is called. And then have self.coadd(outfile) be the calling sequence, to make it clear that this coadds and writes to disk.

This is a good idea, but there is also a method here that writes out separate datacubes (when align=True and combine=False). I've just realised that this is confusing, given that the routine is called "coadd"... So I've renamed the routine to run(), which I think is more consistent with the rest of PypeIt anyway.

It has made me realise though that this routine is more about making datacubes, and not necessarily "coadding"... Coadding is just one possible outcome. So I wonder if this might be confusing, and we should name it something else (perhaps spec3dobj is more apt)?

pypeit/coadd3d.py

jhennawi · 2023-10-08T10:38:46Z

pypeit/coadd3d.py

+
+        # If the user is aligning or combining, the spatial scale of the output cubes needs to be consistent.
+        # Set the spatial and spectral scales of the output datacube
+        self.set_voxel_sampling()


See my comments above, set_voxel_sampling should return something. As a general rule, control flows like

self.do_x()
self.do_y()
self.do_z()

Are not only inscrutable, but often do not manage variable scope and are prone to things changing accidentally changing behind the scenes, like if x changes variables that are then required for y. I know that everything is an attribute of the object, but actually that should not really be the case. The fewer global variables the better.

Yep - good catch - as mentioned above, I've moved this function to core.

jhennawi

Thanks for the major refactor @rcooke-ast. I have only a few relatively minor comments. Only one place self.compute_weights, where I think it makes more sense for a method to be in core. Many thanks!

rcooke-ast

OK, I think that addresses all comments of @jhennawi and @kbwestfall. I'll run the dev suite again to make sure I haven't broken anything in the process!

pypeit/coadd3d.py

rcooke-ast · 2023-10-08T12:17:41Z

pypeit/coadd3d.py

+            msgs.info("Adopting a square pixel spatial scale of {0:f} arcsec".format(3600.0 * self._dspat))
+        # If the user has not specified the spectral sampling, then set it now to the largest value
+        if self._dwv is None:
+            self._dwv = np.max(self._specscale)


Good catch - I agree 👍 Actually, I decided to move this entire section of code to core.

rcooke-ast · 2023-10-08T12:47:08Z

pypeit/coadd3d.py

+        produce a spec3d file, which is a DataCube representation of a PypeIt spec2d frame for SlicerIFU data.
+        """
+        # Initialise variables
+        wave_ref = None


Sounds good - If I understand correctly, I've added this information to the docstring.

rcooke-ast · 2023-10-08T12:50:36Z

pypeit/coadd3d.py

+
+        # If the user is aligning or combining, the spatial scale of the output cubes needs to be consistent.
+        # Set the spatial and spectral scales of the output datacube
+        self.set_voxel_sampling()


Yep - good catch - as mentioned above, I've moved this function to core.

pypeit/coadd3d.py

rcooke-ast · 2023-10-08T12:54:18Z

pypeit/coadd3d.py

+            sensfunc = self.flux_spline(senswave)
+
+        # Generate a datacube
+        outfile = datacube.get_output_filename("", self.cubepar['output_filename'], True, -1)


This is a good idea, but there is also a method here that writes out separate datacubes (when align=True and combine=False). I've just realised that this is confusing, given that the routine is called "coadd"... So I've renamed the routine to run(), which I think is more consistent with the rest of PypeIt anyway.

It has made me realise though that this routine is more about making datacubes, and not necessarily "coadding"... Coadding is just one possible outcome. So I wonder if this might be confusing, and we should name it something else (perhaps spec3dobj is more apt)?

rcooke-ast · 2023-10-08T20:36:12Z

pypeit/coadd3d.py

+        Compute the relative weights to apply to pixels that are collected into the voxels of the output DataCubes
+        """
+        # Calculate the relative spectral weights of all pixels
+        if self.numfiles == 1:


I do agree, and I started to rework this, but then realised that this routine calls generate_image_subpixel, which is located at the bottom of this coadd3d.py file (outside of this class). It required a big change throughout a lot of the code (and avoid circular imports), and I've now moved considerably more code over to core (for better or worse).

pypeit/coadd3d.py

rcooke-ast · 2023-10-09T10:47:40Z

pypeit/coadd3d.py

+        Compute the relative weights to apply to pixels that are collected into the voxels of the output DataCubes
+        """
+        # Calculate the relative spectral weights of all pixels
+        if self.numfiles == 1:


This was a painful change, but I think it's all there now.

rcooke-ast · 2023-10-09T21:42:38Z

Tests passing...

kbwestfall

I had a couple of lingering comments/responses, but I'm happy to approve now.

rcooke-ast

Thanks again @kbwestfall - all comments addressed... I think!

rcooke-ast · 2023-10-10T18:54:02Z

pypeit/coadd3d.py

+                msgs.info("Loading skysub frame:" + msgs.newline() + opts_skysub)
+                try:
+                    spec2DObj_sky = spec2dobj.Spec2DObj.from_file(opts_skysub, self.detname)
+                    skysub_exptime = fits.open(opts_skysub)[0].header['EXPTIME']


Yes, that is a good point - it's included in the header 🤦‍♂️ so I've done that now... thanks for spotting that! Fixed throughout.

rcooke-ast · 2023-10-10T19:04:42Z

pypeit/coadd3d.py

+            msgs.info("Spatial shift of cube #{0:d}:" + msgs.newline() +
+                      "RA, DEC (arcsec) = {1:+0.3f} E, {2:+0.3f} N".format(ff + 1,
+                                                                           self.opts['ra_offset'][ff],
+                                                                           self.opts['dec_offset'][ff]))


Sorry - I somehow missed this during the first pass!

This offset is usually something that the user sets at the telescope (i.e. it's usually a dither or sky offset - Like moving the telescope "5 arcsec East and 2 arcsec North", which already includes the cos(dec) factor). I think this is more practical than specifying two separate RA/Dec combinations (which is what appears to be needed by the SkyOffsetFrame).

Perhaps I need to clarify the docs on this?

rcooke-ast · 2023-10-11T13:01:54Z

pypeit/coadd3d.py

+        """
+        Utility function to provide the world coordinate system of the datacube
+        """
+        return wcs.WCS(self.head0)


Ahh, I think I see. No one will actually no about self._ivar, it will just be a convenience variable so that when the @property ivar is called, it doesn't (necessarily) compute the 1/sig**2. I think this is a reasonable compromise. I've implemented the change you have suggested, and I initialise self._ivar=None. Same for the WCS

jhennawi

Thanks for all the hard work here @rcooke-ast

rcooke-ast added 18 commits September 8, 2023 11:00

SlicerIFU

3f3773d

rm coadd

a60473f

refactor to coadd3d

6b33349

refactor step 1

79a9cb4

refactor step 2

b1b7808

deprecate unused

39a1e8d

restructure

8aae909

restructured

97f695a

update docstrings

ed9169b

more docstrings

7b17e41

more docstrings

dafc1be

more docstrings

2871ebf

final docstrings

dcbd362

final docstrings

78b323c

refactor

fe055ee

fix args

4e7db8f

log update

e6b652c

renamed datacube

2b045bc

Merge branch 'develop' into datacube_core

f7597c5

jhennawi self-requested a review September 11, 2023 21:07

rcooke-ast added 2 commits September 12, 2023 09:51

fix tests

5f657bb

voxel sampling

cd63beb

rcooke-ast mentioned this pull request Sep 13, 2023

Datacube core pypeit/PypeIt-development-suite#287

Merged

rcooke-ast added 6 commits September 13, 2023 14:39

update comment

60884e9

make a start

a7d4e3a

wrapper for create_wcs

d8f0f55

Merge branch 'datacube_core' into hst_align_18

d6fbff2

moving forward

c033320

fixes

f7fa87d

rcooke-ast mentioned this pull request Oct 7, 2023

Avoid transposing when saving DataCubes #1699

Open

rcooke-ast added 2 commits October 8, 2023 07:04

cleanup DAR

07c7354

Merge branch 'develop' into datacube_core

982eea1

rcooke-ast commented Oct 8, 2023

View reviewed changes

jhennawi reviewed Oct 8, 2023

View reviewed changes

pypeit/coadd3d.py Show resolved Hide resolved

jhennawi reviewed Oct 8, 2023

View reviewed changes

jhennawi requested changes Oct 8, 2023

View reviewed changes

rcooke-ast mentioned this pull request Oct 8, 2023

Make a script that will calculate the sensitivity function for DataCubes. #1701

Closed

rcooke-ast added 11 commits October 8, 2023 20:52

code cleanup

5ac775b

move to core

6754e32

wcs to core

577822f

bpm spatflip

7eba69e

bpm spatflip

f1599cf

update docstring

4e50f30

wcs fix

270eaf1

rm todo

75dbe33

weights to core

d979e19

docstring

e330ec2

cleanup

dfa747f

rcooke-ast commented Oct 9, 2023

View reviewed changes

cleanup

b1d6e1f

kbwestfall approved these changes Oct 10, 2023

View reviewed changes

cleanup

3ad7d3c

rcooke-ast commented Oct 11, 2023

View reviewed changes

jhennawi approved these changes Oct 14, 2023

View reviewed changes

rcooke-ast merged commit 0c03daf into develop Oct 14, 2023
23 checks passed

rcooke-ast deleted the datacube_core branch October 14, 2023 19:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Datacube core #1669

Datacube core #1669

rcooke-ast commented Sep 11, 2023 •

edited

Loading

codecov-commenter commented Sep 11, 2023 •

edited

Loading

rcooke-ast left a comment

rcooke-ast Oct 4, 2023

jhennawi Oct 8, 2023

rcooke-ast Oct 8, 2023

jhennawi Oct 8, 2023

rcooke-ast Oct 8, 2023

jhennawi Oct 8, 2023

rcooke-ast Oct 8, 2023

jhennawi left a comment

rcooke-ast left a comment

rcooke-ast Oct 8, 2023

rcooke-ast Oct 8, 2023

rcooke-ast Oct 8, 2023

rcooke-ast Oct 8, 2023

rcooke-ast Oct 8, 2023

rcooke-ast Oct 9, 2023

rcooke-ast commented Oct 9, 2023

kbwestfall left a comment

rcooke-ast left a comment

rcooke-ast Oct 10, 2023

rcooke-ast Oct 10, 2023

rcooke-ast Oct 11, 2023

jhennawi left a comment

Datacube core #1669

Datacube core #1669

Conversation

rcooke-ast commented Sep 11, 2023 • edited Loading

codecov-commenter commented Sep 11, 2023 • edited Loading

Codecov Report

rcooke-ast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhennawi left a comment

Choose a reason for hiding this comment

rcooke-ast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcooke-ast commented Oct 9, 2023

kbwestfall left a comment

Choose a reason for hiding this comment

rcooke-ast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhennawi left a comment

Choose a reason for hiding this comment

rcooke-ast commented Sep 11, 2023 •

edited

Loading

codecov-commenter commented Sep 11, 2023 •

edited

Loading