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-22550: Fix some header translations for LATISS data #152

Merged
merged 16 commits into from Dec 17, 2019
Merged

Conversation

timj
Copy link
Member

@timj timj commented Dec 13, 2019

@tribeiro, @patrickingraham Can you please take a look at this. It looks like a large review but in reality it's fairly small. Please focus on the changes in latiss.py and filters.py.

There are correction files that fix the disperser->diffuser filter problem.

The important things are:

  • What should we have for the LATISS filters?
  • Are my corrections in fix_header reasonable?
  • Are you okay with the fixes to observation_type and physical_filter?

Finally, should I try to set DARKTIME to EXPTIME if it dark time is 0.0s but exp time is > 0?

Requires lsst/obs_base#190

@timj timj requested a review from tribeiro December 13, 2019 17:51
python/lsst/obs/lsst/filters.py Outdated Show resolved Hide resolved
python/lsst/obs/lsst/filters.py Outdated Show resolved Hide resolved
python/lsst/obs/lsst/filters.py Show resolved Hide resolved
python/lsst/obs/lsst/translators/latiss.py Outdated Show resolved Hide resolved
python/lsst/obs/lsst/translators/lsst.py Show resolved Hide resolved
This is much shorter and removes the noqa that was masking
a real issue.
An explicit function definition is clearer than lambda in this
situation and does not require the noqa
Do not assign to a variable we are not going to use.
Patrick Ingraham feels that this header will never give a
useful value for LATISS.
Now force dark time to be the exposure time any time
that the dark time is lower than the exposure time.
Otherwise we aren't really sure which observation is
causing the problem.
Copy link

@patrickingraham patrickingraham 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 to me now and fulfills requests.

@timj timj merged commit 79942e9 into master Dec 17, 2019
@timj timj deleted the tickets/DM-22550 branch December 17, 2019 17:58
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