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

DM-39649: Add default LATISS transmission_detector and transmission_optics. #13

Merged
merged 6 commits into from Jul 14, 2023

Conversation

erykoff
Copy link
Contributor

@erykoff erykoff commented Jul 11, 2023

No description provided.


This was then converted to the calib format by running the script `python $OBS_LSST_DIR/python/lsst/obs/lsst/script/rewrite_latiss_qe_file.py`.

Both the original FITS and output ecsv files have been committed to this repo.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe confirm that it's the ECSV version that is the one used by the calibration system?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you worried the other files there might be picked up? Should they go in a different directory? But the calibration ecsv file is the one that is used, and it is the only one with the correct name. (On jira I have a script that shows that the files are working as intended).

Copy link
Member

Choose a reason for hiding this comment

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

Is there a downside to an explicit comment? It might be that it would be best to put the original source files somewhere else. It does occur to me as well that it might be nice to add provenance to the ECSV file indicating where the data came from.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I don't quite understand what you're suggesting.

Copy link
Member

Choose a reason for hiding this comment

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

Which part?

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding a keyword is a good idea, although we've already been inconsistent on this. I think my suggestion here is going to be to choose any reasonable key name to add the source file name to the newly added ECSV files. As this ticket already has edits on the LATISS transmission_filter files, those should be updated to that same keyword (it's just SOURCE now).

The FITS file is safe from being picked up as we restrict the inputs in this package to be one of {.ecsv, .yaml, json}, so there's no reason to move them out of the way now. I think moving them to a _src directory makes sense, but I don't have a clear idea of where that directory should live (do we have one top level _src containing all of our sources, do we have one for each instrument, or do we have one for each calibration type), so I propose adding a "clean up" ticket that can handle that decision, do the moves, and enforce whatever keyword choices onto the existing files without making this ticket any bigger.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So should I just add a SOURCE key to each of these ecsv files? That makes sense to me.

Copy link
Member

Choose a reason for hiding this comment

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

Okay. At some point we should make the rewrite script do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, that script is on this ticket (in obs_lsst), so that's where I was going to put it. It's simply a question of the name of the key.

Copy link
Contributor

Choose a reason for hiding this comment

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

SOURCE works for me.

Copy link
Contributor

@czwa czwa left a comment

Choose a reason for hiding this comment

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

A few questions throughout, and a request to add some header keyword denoting the sources to the files that will be ingested as curated calibrations.

@@ -19,7 +19,7 @@
# - __serialized_columns__:
# throughput:
# __class__: astropy.units.quantity.Quantity
# unit: !astropy.units.Unit {unit: ''}
# unit: !astropy.units.Unit {unit: ""}
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this remove the "unknown unit" issue, or is this just switching to a consistent quote style?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This removes the unknown unit issue, strangely.

Visual inspection shows that the AuxTel mirror reflectivity at the 7 sampled wavelengths is on average consistent with the Subaru mirror as scanned on 19 Feb, 2020.
The file "subaru_m1_r_20200219.txt" was retrieved from https://www.naoj.org/Observing/Telescope/Parameters/Reflectivity/subaru_m1_r_20201209.txt on 10 July 2023, and then edited to remove some erroneous spaces that made it so Astropy was unable to read it properly, as well as the second measurements between 950 and 1000 nm.

The final mirror transmission used as the AuxTel baseline is then taken as the Subaru M1 measurement from 19 February, 2020, and adjusted by 3.5% to match the average of the throughput measurements.
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this fit done for a particular date, or was the full ensemble of AuxTel measurements used to determine the scale factor to match the Subaru measurement?
Are planning to have our own high resolution reflectivity scan in the future? I'm concerned that the mirror degradation between AuxTel and Subaru will be different, as I would expect Subaru to have a higher rate of volcanic dust deposition than AuxTel.

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've updated the text:

The final mirror transmission used as the AuxTel baseline is then taken as the Subaru M1 measurement from 19 February, 2020, and adjusted by 3.5% to match the average of the AuxTel throughput measurements over 3 years of measurements.
The chromatic shape of the mirror transmssion recorded in this repo is then consistent with the average AuxTel measurements, but at higher resolution, including the complex shape of the reflectivity of aluminum around 800 nm that are not captured in the AuxTel scans.
For calibration purposes, the chromatic shape of the mirror reflectivity is much more important than the absolute throughput which is degenerate with numerous other effects.

And interestingly it does not look like the chromatic shape of the AuxTel mirror reflectivity changed very much over the few years that the scanning has been done.

The final mirror transmission used as the AuxTel baseline is then taken as the Subaru M1 measurement from 19 February, 2020, and adjusted by 3.5% to match the average of the throughput measurements.
The mirror transmssion recorded in this repo is then consistent with the AuxTel measurements, but at higher resolution, including the rapid changes around 800 nm that are not captured in the AuxTel scans.

Note that this file only contains reflectivity from AuxTel M1, and not from any of the other optical elements.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is another concern. I think a note on the JIRA ticket that either details how we'll update these in the future or stating that a plan needs to be made (with an additional ticket for that work?) would be good so it's clear if/when these will be updated.

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 think the medium-term plan for updating into the future would be to make use of the in-situ measurements of the system throughput using the monochrometer + photodiode + fiber spectrograph. I'm not sure how this is all ticketed and planned, though.

4/27/23,85.18,86.05,87.25,88.00,87.55,85.50,91.58
5/12/23,86.07,87.03,88.20,88.17,87.67,86.13,91.27
5/18/23,85.68,86.55,87.55,88.33,87.68,85.53,91.53
5/29/23,85.20,86.63,87.67,86.97,86.87,84.83,90.53
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing final newline.

384 80.78 81.20 81.46 82.19 81.83
382 81.51 81.40 81.89 81.95 81.26
380 81.63 82.02 81.34 80.81 81.44

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a significant gap, or is it just a result of the different wavelength ranges? The README states that this has already been edited for astropy legibility, so I just wanted to check that there are no nan nan nan nan nan nan rows being populated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It just skips those rows, but I can delete them because I've already edited the file.


This was then converted to the calib format by running the script `python $OBS_LSST_DIR/python/lsst/obs/lsst/script/rewrite_latiss_qe_file.py`.

Both the original FITS and output ecsv files have been committed to this repo.
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding a keyword is a good idea, although we've already been inconsistent on this. I think my suggestion here is going to be to choose any reasonable key name to add the source file name to the newly added ECSV files. As this ticket already has edits on the LATISS transmission_filter files, those should be updated to that same keyword (it's just SOURCE now).

The FITS file is safe from being picked up as we restrict the inputs in this package to be one of {.ecsv, .yaml, json}, so there's no reason to move them out of the way now. I think moving them to a _src directory makes sense, but I don't have a clear idea of where that directory should live (do we have one top level _src containing all of our sources, do we have one for each instrument, or do we have one for each calibration type), so I propose adding a "clean up" ticket that can handle that decision, do the moves, and enforce whatever keyword choices onto the existing files without making this ticket any bigger.

# - {CALIBDATE: '1970-01-01T00:00:00'}
# - {INSTRUME: LATISS}
# - {OBSTYPE: transmission_optics}
# - {CALIBCLS: lsst.ip.isr.IntermediateOpticsTransmissionCurve}
Copy link
Contributor

Choose a reason for hiding this comment

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

Pulling from the discussion below: this file also should have a keyword noting the data source.

# - {INSTRUME: LATISS}
# - {OBSTYPE: transmission_sensor}
# - {DETECTOR: 0}
# - {CALIBCLS: lsst.ip.isr.IntermediateSensorTransmissionCurve}
Copy link
Contributor

Choose a reason for hiding this comment

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

And a source keyword here, just for completeness.

@erykoff erykoff merged commit 5736ae6 into main Jul 14, 2023
3 checks passed
@erykoff erykoff deleted the tickets/DM-39649 branch July 14, 2023 21:17
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.

None yet

3 participants