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-43436: Enable OBJECT and AZEL translations for LSSTCam #517

Merged
merged 8 commits into from Mar 26, 2024

Conversation

timj
Copy link
Member

@timj timj commented Mar 23, 2024

ComCamSim needs special code because the AZEL values are missing and airmass is inconsistent with the observation date.

Phosim comcam observations seem to be missing AZSTART so that
has to be skipped.
This is currently problematic because the simulated files
use a DATE-BEG that differs from the date used to calculate
the airmass and hour angle. It's not clear what the right
thing to do is for azel calculation since calculating
the actual values for the DATE-BEG might end up with
a negative elevation. Trying to calculate the azimuth from the
hour angle / declination / airmass would lead to an azimuth
that is not consistent with the time and ra/dec.
Must disable WCS checks because AltAz is inconsistent.
@timj timj requested a review from jchiang87 March 23, 2024 01:41
@timj
Copy link
Member Author

timj commented Mar 23, 2024

It has been suggested on Slack that I also add header fixup code for comcamsim to copy RA/DEC into [RA|DEC][START|END} since they must be identical in simulated data like this.

This doesn't solve the AZEL problem but does mean that downstream of ISR the header will be filled in.

Copy link
Contributor

@jchiang87 jchiang87 left a comment

Choose a reason for hiding this comment

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

Looks good, though I think the controller code test for simulated data should include "S".

controller = self._header[ctrlr_key]
self._used_these_cards(ctrlr_key)
controller = self._get_controller_code()
if controller:
if controller in "HPQ":
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this test also include controller "S"?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jchiang87 I fixed this but I also added some header fixups for things like RASTART/RAEND and fixed an age-old ComCam translator bug from #167. It's quite a lot of code so I've asked for a re-review.

The fix from DM-23310 (6325206)
was incorrect since it was checking for a telescope match when
it should have been checking for a "LSST_CAMERA" instrument
name. This meant that the ComCam translator was saying it could
translate ComCamSim headers.

Also removed the broken check from ComCamSim since two
instruments can't be using identical (and in this case incorrect)
heuristics for determining whether they can translate the header.
@timj timj requested a review from jchiang87 March 25, 2024 20:17
@timj
Copy link
Member Author

timj commented Mar 25, 2024

I will fix the problem with the day_obs offset calculation on another ticket. It's not important for ops rehearsal since we are not yet using butler universe version 6 for that.

Copy link
Contributor

@jchiang87 jchiang87 left a comment

Choose a reason for hiding this comment

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

lgtm.

@timj timj merged commit 7388221 into main Mar 26, 2024
3 checks passed
@timj timj deleted the tickets/DM-43436 branch March 26, 2024 04:55
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

2 participants