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

dc_to_pe should be pe_to_dc in SimTelEventSource and MCCameraEventContainer #1164

Open
kosack opened this issue Oct 30, 2019 · 6 comments
Open

Comments

@kosack
Copy link
Contributor

kosack commented Oct 30, 2019

Looking at the values used for the dc_to_pe value used in the MC (and how they are used), I think the name is backwards:

gain = mc.dc_to_pe[..., np.newaxis]
r1.waveform = (adc_samples - ped) * gain

Looking at the values for the high and low-gain channels:

In [5]: event.mc.tel[3].dc_to_pe.mean(axis=1)
Out[5]: array([0.00611202, 0.11115392], dtype=float32)

And since we multiple by gain (and the values are verified to be <1), it means this is the PE/DC ratio, not the other way around. So we should probably change the name to avoid confusion!

This could be cleaned up when the rest of the calibration is cleaned up, i.e. in #1160 (note that it seems the dc_to_pe ratio is in quite a few containers, and should really go only in one place, whether it is loaded from the MC file or from a real computation)

@maxnoe
Copy link
Member

maxnoe commented Oct 30, 2019

Maybe I'm missing something here because I'm not a native speaker, but it converts dc to pe, or not? So the naming is correct?

@kosack
Copy link
Contributor Author

kosack commented Oct 30, 2019

Ah, I guess that could be interpreted that way, but normally when you use "to", it's the ratio "DC/PE"

@kosack
Copy link
Contributor Author

kosack commented Oct 30, 2019

I guess that means we should really be more specific: is it the DC → PE conversion factor, or the DC/PE ratio (I assumed the second, which is how it's defined in most literature, but clearly it could be either!).

Normally, I like to be able to do unit analysis, so I see (something in DC) * (PE/DC) = (something in PE)

@kosack
Copy link
Contributor Author

kosack commented Oct 30, 2019

we could of course define an astropy unit for PE and DC, but that could be overkill

@watsonjj
Copy link
Contributor

we could of course define an astropy unit for PE and DC, but that could be overkill

I think this is actually a good idea, as long as we remove the units before applying to the waveforms, and it doesn't impact speed. For certain we should make it clear in the description.

Note, I don't intend to move applying this factor to the DL1 calibration. This is only the correct correction factor If you integrated the full signal (without any NSB included). It is not the correct factor when only integrating a part of the signal (with a limited integration window). I would suggest keeping this calibration where it is. It is an R1 calibration to get the waveform into ~pe and flat-fielded. The equivalent for real camera data would be handled by each camera server before the data is at the R1 level.

@kosack
Copy link
Contributor Author

kosack commented Nov 21, 2019

perhaps we should always write this as pe_per_dc for (PE/DC) or dc_per_pe (DC/PE) since it's clear "to" could mean either "divided by" or "conversion to" ambiguously.

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

No branches or pull requests

3 participants