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

Update obs_comCam to work with brighter-fatter processing #14

Merged
merged 1 commit into from Oct 11, 2018

Conversation

mfisherlevine
Copy link
Contributor

No description provided.

Copy link
Contributor

@r-owen r-owen left a comment

Choose a reason for hiding this comment

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

One requested change and one question.

config/bf.py Outdated
config.isr.doUseOpticsTransmission = False
config.isr.doUseFilterTransmission = False
config.isr.doUseSensorTransmission = False
config.isr.doUseAtmosphereTransmission = False
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the task DefaultName and thus this file name should be "measureBrighterFatter" or "measureBrighterFatterKernel".

Also, this is a lot of overrides...can these config parameters be set as defaults by the "measure brighter fatter" task itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call on the config filename, thanks. I don't think the defaults can necessarily be set in the task itself, that seems like dangerous territory, but they are checked to be correct if the keys are present in the config dict; I hope this will suffice (and this is the same for all packages).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is going to be something we see more and more though, because calibration product generation necessarily wants to invoke some bits of isr but not others, so the default config will rarely be appropriate. I think that if this is not OK then we should leave it as-is for now, but talk about that larger issue in an appropriate place, but let's cross that bridge when we get to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having updated the filename I'll assume I'm OK to merge unless you shout.

@RobertLuptonTheGood
Copy link
Contributor

RobertLuptonTheGood commented Sep 17, 2018 via email

Copy link
Contributor

@r-owen r-owen 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

@mfisherlevine mfisherlevine merged commit cb0fd33 into master Oct 11, 2018
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