Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Spectrum objects for obs_fiberSpectrograph #916

Merged
merged 1 commit into from
Dec 5, 2023
Merged

Conversation

aferte
Copy link
Contributor

@aferte aferte commented Nov 30, 2023

We are only doing the merge for daf_butler now and will do the obs package later.

@aferte aferte requested a review from timj November 30, 2023 22:30
@@ -234,6 +234,8 @@ storageClasses:
pytype: lsst.ip.isr.CrosstalkCalib
Linearizer:
pytype: lsst.ip.isr.Linearizer
Spectrum:
Copy link
Member

@timj timj Nov 30, 2023

Choose a reason for hiding this comment

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

I don't like the name of this. Spectrum sounds like a generic class whereas this is specifically a spectrum from the fiber spectrograph, which doesn't sound like it's a Spectrum that a different spectrograph would use. Maybe it is, but then it's probably in the wrong place. Can you please give me some guidance? Otherwise FiberSpectrum would be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see - indeed I believe we only expect this from the fiber spectrograph. Would it be ok to replace it by fiberSpectroData?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or rather fiberSpecData as FiberSpec is the name of the instrument in the obs package.

Copy link
Member

Choose a reason for hiding this comment

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

Convention is for upper case first letter.

"Data from the fiber spectrograph" seems like it might mean the raw. That's why I was edging towards FiberSpectrum as in spectrum from fiber. FiberSpecSpectrum might work if you don't mind doubling up "Spec".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good - done in daf_butler and I will update the obs package accordingly later on this week.

Copy link

codecov bot commented Nov 30, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (bfad270) 87.31% compared to head (9bdaa3b) 87.31%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #916   +/-   ##
=======================================
  Coverage   87.31%   87.31%           
=======================================
  Files         286      286           
  Lines       37168    37168           
  Branches     7833     7833           
=======================================
  Hits        32452    32452           
  Misses       3519     3519           
  Partials     1197     1197           

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

Add Spectrum objects for obs_fiberSpectrograph

Update name of spectrum.
@aferte aferte merged commit 697be36 into main Dec 5, 2023
17 checks passed
@aferte aferte deleted the tickets/DM-40740 branch December 5, 2023 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants