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-29344: Investigate the CI differences between Gen 2 and 3 in COSMOS field (continued) #13

Closed
wants to merge 1 commit into from

Conversation

mrawls
Copy link
Contributor

@mrawls mrawls commented May 4, 2021

This PR removes the line in config/isr.py which explicitly turned off the brighter-fatter correction (in gen2) for this dataset. The goal is to match the gen3 results, which currently has brighter-fatter on by default.

@mrawls mrawls requested a review from isullivan May 4, 2021 02:39
@@ -1,7 +1,6 @@
# Config override for lsst.ip.isr.IsrTask

# Can't ingest the necessary datasets
config.doBrighterFatter = False
config.doAttachTransmissionCurve = False
config.doUseOpticsTransmission = False
config.doUseFilterTransmission = False
Copy link
Member

@kfindeisen kfindeisen May 4, 2021

Choose a reason for hiding this comment

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

Given the comment above (and the assumption that removing the line doesn't break things), I wonder how many of the other lines are also unneccessary. It looks like we weren't correctly handling curated calibs when this file was written (i.e., before lsst/ap_verify#91).

Edit: the underlying problem is that the way obs_subaru handled these datasets in Gen 2 was very non-standard (because there was no standard), and ap_verify can't handle the necessary datasets without special-casing.

Copy link
Member

@kfindeisen kfindeisen May 4, 2021

Choose a reason for hiding this comment

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

EDIT: the Gen 2/3 discrepancy is actually a bug in ap_verify; it hard-codes the files it can load in Gen 3, and doesn't load isr.py. It should be resolved by DM-26140.

@mrawls
Copy link
Contributor Author

mrawls commented May 4, 2021

After discussion with Krzysztof, we don't actually want to turn this back on here, because it won't work. Going to close this PR and open one for ap_verify instead.

@mrawls mrawls closed this May 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants