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-17866: Add support for BOT/lsstCam #87

Merged
merged 7 commits into from Mar 21, 2019
Merged

DM-17866: Add support for BOT/lsstCam #87

merged 7 commits into from Mar 21, 2019

Conversation

timj
Copy link
Member

@timj timj commented Mar 19, 2019

No description provided.

@timj timj marked this pull request as ready for review March 20, 2019 20:45
Copy link
Member

@RobertLuptonTheGood RobertLuptonTheGood left a comment

Choose a reason for hiding this comment

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

My main comment is that I can't figure out the scope of this set of changes, and would like a the original Jira ticket to be clearer! From off-line discussion with @timj I believe that this PR makes it possible to ingest BOT data as lsstCam, and its the job of someone else (probably @rhl) to change the lsstCam yaml files to reflect the current state of the camera at SLAC.
Two minor in-line comments

# Do not yet have a header explicitly for this.
program = None
if "TSTAND" in self._header and "DAYOBS" in self._header:
program = self._header["DAYOBS"]

Choose a reason for hiding this comment

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

Can we set program to None or "unknown" or something rather than DAYOBS? I don't think that program has any LSST meaning, and we shouldn't be relying on it.

Copy link
Member Author

Choose a reason for hiding this comment

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

This gets converted to gen2 run concept and ingest fails if run is None so I have to pick something. LSST does have a science program meaning since we will be distinguishing between all the deep drilling fields and fast/wide/deep. This definition of run is consistent with what is happening in HSC translation.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll switch to "unknown" (we used dayObs for UCD translator).

for filename, expected in test_data:
with self.subTest(f"Testing {filename}"):
self.assertObservationInfoFromYaml(filename, dir=self.datadir, **expected)

Choose a reason for hiding this comment

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

What is this testing? On the face if it, that we can read ordered dicts from yaml but there must be something more. A doc string would help

Copy link
Member Author

Choose a reason for hiding this comment

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

This is standard boiler plate for astro_metadata_translator testing. The raw headers are read in and translated and the results of the translation are compared with the supplied dict.

In particular the bulk of lsstsim.py has been moved to lsst.py
since it is not simulator-specific code.
LsstCamMapper is now a subclass of LsstBaseMapper.  This allows
the specific translation classes to be specified.
Also remove testType since it's the same as imgType.
@timj timj merged commit 00d4fc8 into master Mar 21, 2019
@timj timj deleted the tickets/DM-17866 branch March 21, 2019 22:50
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