Coadd1d bad spec1d #1646

debora-pe · 2023-08-03T23:54:30Z

This add a new functionality in coadd1d to reject (i.e., not coadd) spec1ds that they are bad extractions. The spec1ds are rejected based on the S/N compared to the typical S/N of all the extractions that are coadded. A new parameter has been added to guide this and is sigrej_exp.
The history in the coadd1d spectra is updated to include the info on the rejected spectra.

jhennawi · 2023-08-08T11:57:27Z

pypeit/core/coadd.py

+    nocoadd_exp = np.array([], dtype=int)
+
+    # check if there are exposures that are completely masked out, i.e., gpms = False for all pixels
+    masked_exp = np.where([np.all(gpm == False) for gpm in gpms])[0]


Get rid of the where and use logical_not

bpm_exp = [np.all(np.logical_not(gpm) for gpm in gpms]

jhennawi · 2023-08-08T12:00:05Z

pypeit/core/coadd.py

+        msgs.warn(f'The following {masked_exp.size} exposure(s) is/are completely masked out. '
+                  f'It/They will not be coadded.')
+        [msgs.warn(f'Exposure {i}') for i in masked_exp]
+        # update the list of exposures that will be coadded


See the code above, no need to use the index i or the where above, i.e. these list comprehensions can be written:

_waves = [wave for (wave, bpm) in zip(waves, bpm_exp) if not bpm_exp]

Likewise for the rest

jhennawi · 2023-08-08T12:05:01Z

pypeit/core/coadd.py

@@ -2156,6 +2159,10 @@ def combspec(waves, fluxes, ivars, gpms, sn_smooth_npix,
        Boolean mask for stacked
        spectrum on wave_stack wavelength grid. True=Good.
        shape=(ngrid,)
+    nocoadd_exp : `numpy.ndarray`_


This should be changed to a list (since the inputs are list) of length next, which is True if the exposure was used and False if the exposure was not used. That is more aligned with the structure of this code.

jhennawi · 2023-08-08T12:05:30Z

pypeit/core/coadd.py

@@ -2164,15 +2171,60 @@ def combspec(waves, fluxes, ivars, gpms, sn_smooth_npix,
    _fluxes = [np.float64(flux) for flux in fluxes]
    _ivars = [np.float64(ivar) for ivar in ivars]

+    # exposures that will not be coadded


Change to a list. You could also call it a exp_gpm or something to make it more clear.

jhennawi · 2023-08-08T12:07:09Z

pypeit/core/coadd.py

+                      f'({_sigrej} sigma above the median S/N) in the stack.')
+            [msgs.warn(f'Exposure {i}') for i in bad_exp]
+            if sigrej_exp is not None:
+                msgs.warn('The above exposure(s) will not be coadded.')


Same feedback applies regarding the list comprehensions.

jhennawi

Thanks for this PR @debora-pe. I've been trying to remove masking from the core routines because it makes the control flow and the amount of code to maintain onerous if the masking is occurring in multiple places. My opinion is that whenever possible, masking should take place outside of core routines, i.e. it is understood that the core routine operates on exposures that have already been vetted. That simplifies the control flow in the core routines considerably. I've already implemented these kind of simplifications in object finding and extraction core methods. For that reason, I don't like to see masking being implemented in core routines here.

The most natural place for the vetting that you are doing is in pypeit_collate_1d (for multisite) or pypeit_flux_setup (for echelle). Another possible place for it would be in the Coadd1d class, i.e. the idea is mask in the classes, but not in core.

A counter argument would be that you need the infrastructure from this code in order to perform this vetting, but I'm not convinced about that. Basically you just need to run:

rms_sn, _ = coadd.sn_weights(fluxes, ivars, gpms, const_weights=True)

To access the rms_sn, and you need to have read in the fluxes, ivars, gpms.

I've commented on the code below, but please remove it from core.coadd.combspec and instead put in the Coadd1d multisite child or in pypeit_collate_1d.

codecov-commenter · 2023-08-15T04:01:45Z

Codecov Report

Merging #1646 (6687db3) into develop (23c1009) will decrease coverage by 0.03%.
The diff coverage is 34.74%.

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

@@             Coverage Diff             @@
##           develop    #1646      +/-   ##
===========================================
- Coverage    40.88%   40.86%   -0.03%     
===========================================
  Files          189      189              
  Lines        43491    43575      +84     
===========================================
+ Hits         17782    17805      +23     
- Misses       25709    25770      +61

Files Changed	Coverage Δ
pypeit/coadd2d.py	`8.77% <0.00%> (-0.10%)`	⬇️
pypeit/core/wavecal/wvutils.py	`50.00% <0.00%> (ø)`
pypeit/scripts/coadd_2dspec.py	`6.54% <0.00%> (ø)`
pypeit/coadd1d.py	`16.57% <4.08%> (-4.14%)`	⬇️
pypeit/history.py	`89.47% <81.39%> (-10.53%)`	⬇️
pypeit/par/pypeitpar.py	`96.07% <100.00%> (+<0.01%)`	⬆️

📢 Have feedback on the report? Share it here.

debora-pe · 2023-08-18T20:10:55Z

Tests pass.

Test Summary
--------------------------------------------------------
--- PYTEST PYPEIT UNIT TESTS PASSED  234 passed, 72 warnings in 315.48s (0:05:15) ---
--- PYTEST UNIT TESTS PASSED  123 passed, 195 warnings in 1550.65s (0:25:50) ---
--- PYTEST VET TESTS PASSED  53 passed, 56 warnings in 837.68s (0:13:57) ---
--- PYPEIT DEVELOPMENT SUITE PASSED 215/215 TESTS  ---
Testing Started at 2023-08-17T14:41:38.922491
Testing Completed at 2023-08-18T00:01:05.178846
Total Time: 9:19:26.256355

pypeit/coadd1d.py

jhennawi · 2023-08-30T11:34:24Z

pypeit/coadd1d.py

+            # set sn_smooth_npix if not provided
+            if self.par['sn_smooth_npix'] is None:
+                # same as in coadd.multi_combspec()
+                sn_smooth_npix = int(np.round(0.1 * (np.sum([np.sum(wave > 1.0) for wave in _waves]) / self.nexp)))


See my feedback on your previous version of this PR. You don't need to assign sn_smooth_pix since that is used for the weights, not for the rms_sn, for which the whole spectrum is used. So all of this sn_smooth_pix should be removed. You should instead write:

rms_sn, _ = coadd.sn_weights(fluxes, ivars, gpms, const_weights=True)

Sorry I missed that. I removed sn_smooth_pix.
Thanks!

jhennawi · 2023-08-30T11:35:32Z

pypeit/coadd1d.py

+            msgs.error('At least 2 exposures are required for coadding.')
+
+        # check if there is any bad exposure by comparing the rms_sn with the median rms_sn among all exposures
+        if len(_fluxes) > 2:


This masking based on a statistical analysis of the rms_sn needs to NOT be the default behavior. My suggestion is that you set sigrej_exp to default to None. And that you only carry out the code below if sigrej_exp is not None.

Masking is not done if sigrej_exp=None. You can see below:

if self.par['sigrej_exp'] is not None: msgs.warn('The above exposure(s) will not be coadded.')

There is still some stats on the S/N, which is printed in the terminal, when sigrej_exp=None. I think it's useful to have that information, but the masking is not applied.

Okay I agree printing the information is useful. I would some text to the warning if sigrej_exp is not None that indicates the exposure is being masked. Right now the user won't know it is masked from watching the terminal outputs.

jhennawi · 2023-08-30T11:38:17Z

pypeit/par/pypeitpar.py

        defaults['sn_smooth_npix'] = None
        dtypes['sn_smooth_npix'] = [int, float]
        descr['sn_smooth_npix'] = 'Number of pixels to median filter by when computing S/N used to decide how to scale ' \
                                  'and weight spectra. If set to None (default), the code will determine the effective ' \
                                  'number of good pixels per spectrum in the stack that is being co-added and use 10% of ' \
                                  'this neff.'

+        defaults['sigrej_exp'] = None


For the default of None, you should not check rms_sn and should not mask in any way.

See my response above.

jhennawi

Hi @debora-pe,

Thanks. The PR look great. I have only minor comments that should be easy to address.

debora-pe · 2023-09-01T21:28:41Z

I re-run the tests, since I haven't done in awhile, and all is good.

Test Summary
--------------------------------------------------------
--- PYTEST PYPEIT UNIT TESTS PASSED  234 passed, 72 warnings in 313.48s (0:05:13) ---
--- PYTEST UNIT TESTS PASSED  123 passed, 195 warnings in 1510.26s (0:25:10) ---
--- PYTEST VET TESTS PASSED  53 passed, 55 warnings in 836.96s (0:13:56) ---
--- PYPEIT DEVELOPMENT SUITE PASSED 215/215 TESTS  ---
Testing Started at 2023-08-31T19:53:35.476086
Testing Completed at 2023-09-01T05:08:37.182021
Total Time: 9:15:01.705935

debora-pe · 2023-09-07T23:28:43Z

merging this.

debora-pe added 9 commits July 21, 2023 11:50

added for multislit

648bbcc

Merge branch 'sensfunc_blaze_jwst_lists' into coadd1d_badspec1d

5e650ee

Merge branch 'sensfunc_blaze_jwst_lists' into coadd1d_badspec1d

c8a27cb

rejection added

79da2ee

Merge branch 'sensfunc_blaze_jwst_lists' into coadd1d_badspec1d

480a075

small fix

7122f14

fix docstring

b6529f7

small change

0d7519d

Merge branch 'develop' into coadd1d_badspec1d

23da51c

debora-pe requested a review from badpandabear August 4, 2023 23:35

badpandabear approved these changes Aug 7, 2023

View reviewed changes

jhennawi reviewed Aug 8, 2023

View reviewed changes

jhennawi requested changes Aug 8, 2023

View reviewed changes

debora-pe added 2 commits August 14, 2023 17:50

joe's comments

3fb2fa2

Merge branch 'develop' into coadd1d_badspec1d

96c21e6

debora-pe added 9 commits August 14, 2023 18:10

change verbose

5e4b6f4

one fix

ef469a1

some coadd2d fixes

71ca995

fix history

f96bc6b

small fix

fb0d77a

small fix

9fdc6f9

typo

1ab4836

fix unit test

9f445b4

Merge branch 'develop' into coadd1d_badspec1d

13e756d

debora-pe added 2 commits August 18, 2023 12:37

some more coadd2d fixes

00495cc

Merge branch 'develop' into coadd1d_badspec1d

8a7ae8d

jhennawi reviewed Aug 30, 2023

View reviewed changes

pypeit/coadd1d.py Outdated Show resolved Hide resolved

jhennawi reviewed Aug 30, 2023

View reviewed changes

jhennawi requested changes Aug 30, 2023

View reviewed changes

debora-pe added 2 commits August 31, 2023 19:38

Merge branch 'develop' into coadd1d_badspec1d

4a03e43

joes comments

5351e1e

jhennawi approved these changes Sep 2, 2023

View reviewed changes

debora-pe added 4 commits September 6, 2023 20:32

last comment

1aec242

Merge branch 'develop' into coadd1d_badspec1d

6b55d6d

last comment

d0a7e90

Merge branch 'develop' into coadd1d_badspec1d

6687db3

debora-pe merged commit 0187781 into develop Sep 7, 2023
23 checks passed

debora-pe deleted the coadd1d_badspec1d branch September 7, 2023 23:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Coadd1d bad spec1d #1646

Coadd1d bad spec1d #1646

debora-pe commented Aug 3, 2023

jhennawi Aug 8, 2023

jhennawi Aug 8, 2023

jhennawi Aug 8, 2023

jhennawi Aug 8, 2023

jhennawi Aug 8, 2023

jhennawi left a comment •

edited

Loading

codecov-commenter commented Aug 15, 2023 •

edited

Loading

debora-pe commented Aug 18, 2023

jhennawi Aug 30, 2023

debora-pe Sep 1, 2023

jhennawi Aug 30, 2023

debora-pe Sep 1, 2023

jhennawi Sep 2, 2023

jhennawi Aug 30, 2023

debora-pe Sep 1, 2023

jhennawi left a comment •

edited

Loading

debora-pe commented Sep 1, 2023

debora-pe commented Sep 7, 2023

Coadd1d bad spec1d #1646

Coadd1d bad spec1d #1646

Conversation

debora-pe commented Aug 3, 2023

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 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Aug 15, 2023 • edited Loading

Codecov Report

debora-pe commented Aug 18, 2023

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 • edited Loading

Choose a reason for hiding this comment

debora-pe commented Sep 1, 2023

debora-pe commented Sep 7, 2023

jhennawi left a comment •

edited

Loading

codecov-commenter commented Aug 15, 2023 •

edited

Loading

jhennawi left a comment •

edited

Loading