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

Do not gain correct data when identifying cosmic rays #712

Closed
wants to merge 3 commits into from

Conversation

mwcraig
Copy link
Member

@mwcraig mwcraig commented Dec 10, 2019

Not sure this is the best fix, but it will get the job done.

@mwcraig mwcraig added this to the 2.0.2 milestone Dec 10, 2019
@MSeifert04
Copy link
Contributor

Do we really want to change the output of a function so drastically in a bug-fix release?

Couldn't we add an "apply_gain" parameter to the function which defaults to True (previous behavior) which can be set to False if the uncorrected image is needed (with your patch)? That way we don't break any code that uses this function and we could (gradually) change the default in the future?

@MSeifert04
Copy link
Contributor

Otherwise the change/test look great. Thanks for tackling this Matt.

@mwcraig
Copy link
Member Author

mwcraig commented Dec 10, 2019

Do we really want to change the output of a function so drastically in a bug-fix release?

I guess it depends on whether we think it is a bug (in which case a bug fix makes sense) or an undocumented feature (in which case we'd be breaking the API).

I'll discuss with @crawfordsm today.

@mwcraig
Copy link
Member Author

mwcraig commented Dec 10, 2019

A couple of comments while I’m thinking about them:

I’m increasingly leaning towards a feature release to fix these things because the current API has issues.

@MSeifert04
Copy link
Contributor

I’m increasingly leaning towards a feature release to fix these things because the current API has issues.

I like that idea.

However there are some additional issues:

  • How do we handle gain if an array is passed in instead of a CCDData.
  • Do we make gain-correction a documented feature of the function, or an optional feature, or do we remove it completely?
  • How do we handle units? Normally plain data is in units of ADU and gain is in e-/ADU (or ADU/e-). However what do we do if one of these assumptions is wrong?

@mwcraig
Copy link
Member Author

mwcraig commented Dec 12, 2019

@crawfordsm and I talked about this extensively a little while ago. I'm going to close this and open a different PR shortly that outlines a more complete approach.

@mwcraig mwcraig closed this Dec 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants