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-33942: Support CURINDEX/MAXINDEX/SIMULATE headers for translation #396

Merged
merged 8 commits into from Jun 1, 2022

Conversation

timj
Copy link
Member

@timj timj commented Apr 22, 2022

config/datasetIngest-gen3.py Outdated Show resolved Hide resolved
@@ -802,3 +802,57 @@ def to_boresight_airmass(self):
log.warning("%s: Unable to determine airmass of a science observation, returning 1.",
self._log_prefix)
return 1.0

@cache_translation
def to_group_counter_start(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

All three of these methods could use a quick docstring.

Copy link
Member Author

Choose a reason for hiding this comment

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

Normally the idea is to read the docstring from MetadataTranslator and we try to have docstring inheritance enabled.

python/lsst/obs/lsst/translators/lsst.py Show resolved Hide resolved
python/lsst/obs/lsst/translators/lsst.py Show resolved Hide resolved
Comment on lines +663 to +664
group_counter_end=349,
group_counter_start=348,
Copy link
Contributor

Choose a reason for hiding this comment

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

This and the next one are the only places where group_counter_end != group_counter_start, and they're only separated by 1. Given that (I think) we are using (or will be using) this to group more than 2 exposures, maybe a test for a wider range would be good? Or am I confusing general groupId stuff with the visit system stuff here?

Copy link
Member Author

Choose a reason for hiding this comment

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

You may want to read the ObservationInfo docs. For LSST instruments these are going to record the first sequence number in the visit and the last sequence number in the visit. Robert insists that there will only ever be 2 exposures in a visit. This is a real LATISS header.

"group_counter_start/end" is kept general enough in ObservationInfo that other instruments can decide to use it for whatever counting scheme they want. I did not want to use the term "visit" in the definition. The LSST define-visits code can define visits by using these group_counters or by using the group_id but we are expecting the group_counter version to be the default.

@timj timj force-pushed the tickets/DM-33942 branch 3 times, most recently from 0e607ea to b956458 Compare June 1, 2022 04:45
@timj timj merged commit ec6c066 into main Jun 1, 2022
@timj timj deleted the tickets/DM-33942 branch June 1, 2022 23:11
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

4 participants