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-39140: add photodiode integration algorithm for use with CCOB #264

Merged
merged 1 commit into from May 22, 2023

Conversation

jchiang87
Copy link
Contributor

This will be used for TS8 and LSSTCam data taken at SLAC for Run 6 testing.

sum : `float`
Total charge measured.
"""
dt = self.timeSamples[:, 1:] - self.timeSamples[:, :-1]
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the indexing here, as I thought both timeSamples and currentSamples were 1-d arrays. Both python and numpy complain about this if I attempt to index a list or a 1-d np.array like this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that these are FITS table inputs, and not the 2-column ascii inputs I've used previously. I think that means there's a missing .flatten() that needs to be added in __init__ on lines 64 and 65 (unless this extra axis is intentional and I'm missing something). If these are flattened, I think this simplifies to the [1:], [:-1] indices that I would have expected.

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 had also thought that the extra axis was intentional, so wrote the code to account for that without flattening the arrays. I think it does make sense to flatten these arrays, but that will have downstream consequences, e.g., places like this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I'll add a separate ticket for that, as it's scope creep to do it here. DM-39338.

# The .currentSamples values are the current integrals over
# the interval preceding the current time stamp, so omit the
# first value.
charge = self.currentScale*self.currentSamples[:, 1:]
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think I understand this scaling. From the two example photodiode files I've looked at, the current samples are all positive, so this inverts them all to be negative. I think that and the min/maxes below inverts the measurements, such that with my test photodiode (instrument='LSSTCam', exposure=3021120700810, collections="LSSTCam/photodiode" as it was the first to return a result from the butler) yields bg_current=-1.19e-7, return=3.50e-7, and np.sum(charge)=-1.12e-6 np.sum(-bg_current*dt)=1.47e-6. The index for my example also is selecting the points where current ~ -1.2e-7, while ~index selects those around -1e-12.
Have I somehow confused myself?

Copy link
Contributor Author

@jchiang87 jchiang87 May 22, 2023

Choose a reason for hiding this comment

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

The data from the CCOB uses a different ammeter than for the BOT data, and the sampled points have negative values for the intervals where the CCOB LED is on, and so differs from the BOT data in that regard. See instrument='LSST-TS8', exposure=202305122056562, collections="LSST-TS8/photodiode" in /repo/ir2.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I was confused about the inputs then, and this makes the default value make more sense to me. The test TS8 photodiode I tried to look at must not have been ingested for some reason.
With a proper example, the values come out looking like I was expecting the first time, so I think that clears my confusion.

@jchiang87 jchiang87 merged commit cba2a62 into main May 22, 2023
1 check passed
@jchiang87 jchiang87 deleted the tickets/DM-39140 branch May 22, 2023 20:46
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