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-42024: Allow for low gains and allow initial outlier cutoff to have units of electrons. #222

Merged
merged 3 commits into from Dec 6, 2023

Conversation

erykoff
Copy link
Contributor

@erykoff erykoff commented Dec 5, 2023

This fixes a bug where low gains (<0.5) were not allowed in the fit. It also now changes maxADUInitialPtcOutlierFit to default of units of electrons that can then be scaled by an initial estimate of the gain. This allows the cutoff to be approximately invariant across a range of gain settings.

@plazas plazas self-requested a review December 5, 2023 21:58
Copy link
Contributor

@plazas plazas left a comment

Choose a reason for hiding this comment

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

LGTM, just a couple of questions.

@@ -1005,8 +1014,8 @@ def errFunc(p, x, y):
if self.config.binSize > 1:
bounds = self._boundsForAstier(parsIniPtc)
else:
bounds = self._boundsForAstier(parsIniPtc, lowers=[-1e-4, 0.5, -2000],
Copy link
Contributor

Choose a reason for hiding this comment

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

How necessary are these bounds for the convergence of this initial fit? In any case, I'm just wondering if even the new values (min=0.2, and max=3.0) will give us trouble in the future. Or if this code is used for other cameras with different settings (I think DECam has gains of 4, so I'm not even sure at this point how this has worked in the past for DECam...)

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 don't think anybody has ever run this on DECam. I'll change this to a bigger range.

Copy link
Contributor

Choose a reason for hiding this comment

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

I did!

if self.config.scaleMaxSignalInitialPtcOutlierFit:
approxGain = np.nanmedian(meanVecOriginal/varVecOriginal)
maxADUInitialPtcOutlierFit = self.config.maxSignalInitialPtcOutlierFit/approxGain
self.log.info(
Copy link
Contributor

Choose a reason for hiding this comment

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

We were concerned in another ticket about too much info in the logs, especially per amp. Will it be OK in this case? Should we change the level to "debug"?

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 did think about this, and could make it debug. But it's not that much. The difference is that this is a log that is done per amp in a PTC fit (16 messages per hundreds of flat pairs) and not a log per amp per ISR (16 messages times hundreds of flat pairs).

@erykoff erykoff merged commit 2f39c65 into main Dec 6, 2023
2 checks passed
@erykoff erykoff deleted the tickets/DM-42024 branch December 6, 2023 16:02
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