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-15862: Reduce ISR code duplication between ip_isr, obs_subaru, and obs_decam #69

Merged
merged 3 commits into from Jan 4, 2019

Conversation

czwa
Copy link
Contributor

@czwa czwa commented Nov 29, 2018

No description provided.

@SimonKrughoff
Copy link
Contributor

As a general comment, it is helpful if the summary line for each commit gives some info about what the commit is doing. Having three commits with identical summaries makes it hard to tell which is which. I really like the detail in the body of the commit BTW.

Copy link
Contributor

@SimonKrughoff SimonKrughoff left a comment

Choose a reason for hiding this comment

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

Thanks very much for the docstring updates. That is really not fun work, but it is so helpful.

See comments. I don't think there is a ton of work here (hopefully).

I really like the reorganization. As most of this is moving things around, I didn't spend a lot of time looking in detail at the algorithmic code. Let me know if that's needed.

@@ -75,7 +75,7 @@ class IsrQaConfig(pexConfig.Config):
doThumbnailOss = pexConfig.Field(
dtype=bool,
doc="Write overscan subtracted thumbnail?",
default=True,
default=False,
Copy link
Contributor

Choose a reason for hiding this comment

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

What jenkins errors does this avoid? Is this in CI, or something else?

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 was triggering obs_ctio0m9 issues. The update to obs_base likely resolved this, so I will do a test with default=True, and see if the error goes away.

python/lsst/ip/isr/isrQa.py Show resolved Hide resolved
python/lsst/ip/isr/isrTask.py Show resolved Hide resolved
python/lsst/ip/isr/isrTask.py Outdated Show resolved Hide resolved
python/lsst/ip/isr/isrTask.py Outdated Show resolved Hide resolved
python/lsst/ip/isr/isrTask.py Outdated Show resolved Hide resolved
python/lsst/ip/isr/isrTask.py Show resolved Hide resolved
@czwa czwa force-pushed the tickets/DM-15862 branch 2 times, most recently from 3e85eb0 to 9458214 Compare December 13, 2018 00:12
The ISR code was split between ip_isr and the obs_ packages, with the
ip_isr and obs_subaru containing parallel ISR reduction pathways.
This ticket merges those pathways into a single set of ISR functions
in the ip_isr package, removing the duplication of code.  The merged
codebase also makes additional useful functions from the obs_subaru
ISR available to other cameras, and the new ISR code now profiles
debug points, thumbnail images, and post-correction statistics to
check processing quality.

A number of new configuration options now exist to further manage ISR
processing.  These allow more control over which operations are
performed.
Additional subtasks have been added to support camera-specific
corrections that are unlikely to share much common code.  These
subtasks manage straylight removal, vignette construction, and
addition masking.  Image statistics have been added in a set of isrQa
methods.
Class and method documentation has been updated across all files to
match the code.
@czwa czwa merged commit b4f2757 into master Jan 4, 2019
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