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-21212: Update existing cp_pipe tasks to pipelineTasks #148

Merged
merged 1 commit into from Jun 9, 2020

Conversation

czwa
Copy link
Contributor

@czwa czwa commented Dec 5, 2019

Add LATISS cp_pipe pipelines.

@czwa czwa requested a review from natelust December 5, 2019 19:17
@czwa czwa force-pushed the tickets/DM-21212 branch 2 times, most recently from 91529ef to 158bcd1 Compare December 17, 2019 20:08
@czwa czwa force-pushed the tickets/DM-21212 branch 2 times, most recently from 47c799f to 782ad59 Compare January 17, 2020 20:22
@czwa czwa force-pushed the tickets/DM-21212 branch 5 times, most recently from 20b122d to ad16882 Compare January 31, 2020 21:09
Copy link

@PaulPrice PaulPrice left a comment

Choose a reason for hiding this comment

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

I would think that most of the contents of these files can be inherited (but I don't know for sure). You should trim as much as you possibly can, as having multiple layers doing the same thing is asking for trouble.

doCrosstalk: False
doDefect: True
doNanMasking: True
doInterpolate: True

Choose a reason for hiding this comment

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

Can most of the above be inherited?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is that it cannot for complexity reasons (inheritance + config setting can be ambiguous/confusing if they both set config parameters). The real fix is in producing CT/Defects/etc for obs_lsst, which would allow the simply inherited version to be used (like obs_subaru).

connections.inputExps: 'cpBiasProc'
connections.outputData: 'biasProposal'
calibrationType: 'bias'
exposureScaling: "ExposureTime"

Choose a reason for hiding this comment

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

You look to be scaling by the exposure time, which should be zero...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a mistake now fixed. It worked because there's a safe guard to prevent scaling by zero, but it's better to ask for the right thing, "None".

config:
connections.inputExps: 'cpBiasProc'
connections.outputData: 'biasProposal'
calibrationType: 'bias'

Choose a reason for hiding this comment

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

Doesn't the above get inherited?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the master cp_pipe pipeline definition? No.
From the camera? Yes, which was one of the issues making this change necessary.

contracts:
- isr.doBias == False
- cpCombine.calibrationType == "bias"
- cpCombine.exposureScaling == "ExposureTime"

Choose a reason for hiding this comment

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

Inherited?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Contracts are not inherited, and are there to ensure command line options don't break what the pipeline is supposed to do.

@czwa czwa force-pushed the tickets/DM-21212 branch 2 times, most recently from 1096901 to daec367 Compare March 4, 2020 22:45
@czwa czwa force-pushed the tickets/DM-21212 branch 2 times, most recently from a9bf4ed to ae12075 Compare May 28, 2020 22:50
@czwa
Copy link
Contributor Author

czwa commented Jun 8, 2020

I've filed a new ticket to handle this issue, https://jira.lsstcorp.org/browse/DM-25329 , in the interest of getting this ancient ticket merged. I'm planning on updating the ci_cpp_latiss code to fully test obs_lsst and LATISS on real data.

@czwa czwa merged commit e18ed1d into master Jun 9, 2020
@timj timj deleted the tickets/DM-21212 branch May 12, 2022 16:48
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