GMOS SOUTH CCD upgrade and some other fixes #1798

debora-pe · 2024-04-12T00:28:39Z

As titled

codecov-commenter · 2024-04-12T00:36:44Z

Codecov Report

Attention: Patch coverage is 20.93023% with 34 lines in your changes are missing coverage. Please review.

Project coverage is 39.05%. Comparing base (4c11a19) to head (f131b07).
Report is 25 commits behind head on develop.

Files	Patch %	Lines
pypeit/spectrographs/gemini_gmos.py	19.04%	34 Missing ⚠️

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

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1798      +/-   ##
===========================================
- Coverage    39.07%   39.05%   -0.02%     
===========================================
  Files          204      204              
  Lines        47012    47046      +34     
===========================================
+ Hits         18370    18375       +5     
- Misses       28642    28671      +29

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

VascoSch92 · 2024-04-13T10:13:38Z

pypeit/spectrographs/gemini_gmos.py

+            if obsepoch is not None:
+                return time.Time(obsepoch, format='jyear').mjd
+            else:
+                return None


you should update the docstring as well, because you are return None instead of a Metadata value read from the header(s)

Agreed. I also wonder if it might be worthwhile throwing an error, or, throw a warning and then assume that the observations have ben taken since "2023-12-14".

We likely do not want it to throw an error because this would cause pypeit_setup to fault, which we want to avoid. Throwing a warning makes sense, and we can add that the return can be None.

Indeed, and I agree, but returning None could cause an error on line 846 (and several other places in this file where a possible None value is not checked)... I think?

Ah, yes, you're right. @debora-pe , you'll need to catch the case @rcooke-ast pointed out. There are other places in the code that can catch a None mjd (e.g., starting at line 255 in metadata.py), but maybe not everywhere.

I decided to use a dummy mjd (using today as date) and throwing a warning. What do you think?

debora-pe · 2024-04-15T21:15:59Z

Tests pass. The failures are unrelated and waiting for another PR to clear.

Test Summary
--------------------------------------------------------
--- PYTEST PYPEIT UNIT TESTS PASSED  249 passed, 64 warnings in 343.40s (0:05:43) ---
--- PYTEST UNIT TESTS FAILED  3 failed, 131 passed, 168 warnings in 1679.39s (0:27:59) ---
--- PYTEST VET TESTS PASSED  61 passed, 97 warnings in 5166.77s (1:26:06) ---
--- PYPEIT DEVELOPMENT SUITE FAILED 1/237 TESTS  ---
Failed tests:
    keck_nirspec/LOW_NIRSPEC-1 pypeit
Skipped tests:
Testing Started at 2024-04-12T17:55:53.048547
Testing Completed at 2024-04-13T07:12:08.495308
Total Time: 13:16:15.446761

jhod0 · 2024-04-16T21:55:15Z

I have been able to successfully run this version of PypeIt on GMOS-South data taken in January!

The resulting spectra seem to be perfectly reasonable, here is a red galaxy targeted by our observations.

Thank you Debora!

rcooke-ast

Just minor comments from me. I'd encourage you to add some recent data to the dev-suite (and I presume there are old data just prior to the detector upgrade already in the dev suite)? Just to make sure that no bugs have crept in for either change.

rcooke-ast · 2024-04-17T15:12:50Z

pypeit/spectrographs/gemini_gmos.py

            )
+
+        # account for the CCD upgrade happened on 2023-12-14
+        if hdu is not None:


Should this perhaps be defined before the detector dictionaries? (i.e. define parameters gain1, gain2, etc...). I think it's fine to update, but might be cleaner if we set variables near the top of this function and use those variables in the dictionary.

I do actually prefer to leave it like this. Other spectrographs have the same flow. Also, the code below is only run if hdu is provided, which would make the code full of if...else if I define the parameters before the dictionary.

rcooke-ast · 2024-04-17T15:13:56Z

pypeit/spectrographs/gemini_gmos.py

+        if hdu is not None:
+            # date upgrade
+            t_upgrade = time.Time("2023-12-14", format='isot')
+            obs_date = time.Time(self.get_meta_value(self.get_headarr(hdu), 'mjd'), format='mjd')


Could this be None sometimes?

See my comment above

rcooke-ast · 2024-04-17T15:15:30Z

pypeit/spectrographs/gemini_gmos.py

+            if obsepoch is not None:
+                return time.Time(obsepoch, format='jyear').mjd
+            else:
+                return None


Agreed. I also wonder if it might be worthwhile throwing an error, or, throw a warning and then assume that the observations have ben taken since "2023-12-14".

VascoSch92 · 2024-04-17T17:10:02Z

pypeit/manual_extract.py

@@ -85,15 +85,15 @@ def by_fitstbl_input(cls, frame: str, inp: str, spectrograph):
        m_es = inp.split(';')
        for m_e in m_es:
            parse = m_e.split(':')
-            # det_strip will be a list of a single number (no mosaic) or 2 numbers (mosaic)
+            # det_strip will be a list of a single number (no mosaic) or >= 2 numbers (mosaic)


If len(det_strip) == 1 the it is not a mosaic, otherwise it is

Just for the comment. In this way it is explicit

kbwestfall

Looks good. My main reason for clicking "request changes" is the detid issue. All of the other comments are minor.

pypeit/spectrographs/gemini_gmos.py

kbwestfall · 2024-04-17T17:31:35Z

pypeit/spectrographs/gemini_gmos.py

+            if obsepoch is not None:
+                return time.Time(obsepoch, format='jyear').mjd
+            else:
+                return None


We likely do not want it to throw an error because this would cause pypeit_setup to fault, which we want to avoid. Throwing a warning makes sense, and we can add that the return can be None.

pypeit/spectrographs/gemini_gmos.py

debora-pe · 2024-04-18T04:06:21Z

@rcooke-ast I agree that it would be good to have data in the Dev Suite, but the CCD update is so knew that there are no available public data. A PypeIt user gave me a couple of frame (no science), but that's everything I have.
And yes, we do have older data in the Dev Suite.

debora-pe · 2024-04-18T22:41:24Z

Tests still good. Except the unrelated failures.

Test Summary
--------------------------------------------------------
--- PYTEST PYPEIT UNIT TESTS PASSED  249 passed, 64 warnings in 346.13s (0:05:46) ---
--- PYTEST UNIT TESTS FAILED  3 failed, 131 passed, 168 warnings in 1660.01s (0:27:40) ---
--- PYTEST VET TESTS PASSED  61 passed, 98 warnings in 5207.49s (1:26:47) ---
--- PYPEIT DEVELOPMENT SUITE FAILED 1/237 TESTS  ---
Failed tests:
    keck_nirspec/LOW_NIRSPEC-1 pypeit
Skipped tests:
Testing Started at 2024-04-17T19:09:40.292404
Testing Completed at 2024-04-18T08:17:22.885371
Total Time: 13:07:42.592967

debora-pe · 2024-04-18T22:41:38Z

Merging this now.

updateCCD+other_fixes

34137da

Merge branch 'develop' into gmoss_ccdupdate

7bc96d0

VascoSch92 reviewed Apr 13, 2024

View reviewed changes

fix manual extraction for mosaic

52c0425

rcooke-ast approved these changes Apr 17, 2024

View reviewed changes

VascoSch92 reviewed Apr 17, 2024

View reviewed changes

kbwestfall requested changes Apr 17, 2024

View reviewed changes

comments

d0420ff

fix detid

f131b07

kbwestfall approved these changes Apr 18, 2024

View reviewed changes

debora-pe merged commit 235a571 into develop Apr 18, 2024
23 checks passed

debora-pe deleted the gmoss_ccdupdate branch April 18, 2024 22:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GMOS SOUTH CCD upgrade and some other fixes #1798

GMOS SOUTH CCD upgrade and some other fixes #1798

debora-pe commented Apr 12, 2024

codecov-commenter commented Apr 12, 2024 •

edited

Loading

VascoSch92 Apr 13, 2024

rcooke-ast Apr 17, 2024

kbwestfall Apr 17, 2024

rcooke-ast Apr 17, 2024

kbwestfall Apr 17, 2024

debora-pe Apr 18, 2024

debora-pe commented Apr 15, 2024

jhod0 commented Apr 16, 2024

rcooke-ast left a comment

rcooke-ast Apr 17, 2024

debora-pe Apr 18, 2024

rcooke-ast Apr 17, 2024

debora-pe Apr 18, 2024

rcooke-ast Apr 17, 2024

VascoSch92 Apr 17, 2024

kbwestfall left a comment

kbwestfall Apr 17, 2024

debora-pe commented Apr 18, 2024

debora-pe commented Apr 18, 2024

debora-pe commented Apr 18, 2024

GMOS SOUTH CCD upgrade and some other fixes #1798

GMOS SOUTH CCD upgrade and some other fixes #1798

Conversation

debora-pe commented Apr 12, 2024

codecov-commenter commented Apr 12, 2024 • edited Loading

Codecov Report

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

debora-pe commented Apr 15, 2024

jhod0 commented Apr 16, 2024

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

kbwestfall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

debora-pe commented Apr 18, 2024

debora-pe commented Apr 18, 2024

debora-pe commented Apr 18, 2024

codecov-commenter commented Apr 12, 2024 •

edited

Loading