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

use "integralGains" for single pulse extraction? #197

Open
dneise opened this issue Jan 19, 2017 · 11 comments
Open

use "integralGains" for single pulse extraction? #197

dneise opened this issue Jan 19, 2017 · 11 comments

Comments

@dneise
Copy link
Member

dneise commented Jan 19, 2017

When extracting the photoncharge we use these individual pixel gain number:
https://github.com/fact-project/fact-tools/blob/master/src/main/resources/default/gain_sorted_20131127.csv
(btw. this is no CSV at all)
They look like this:
integral_gains_from_file

I was wondering if we should also use these numbers in the single pulse extractor, here:
https://github.com/fact-project/fact-tools/blob/master/src/main/java/fact/extraction/SinglePulseExtraction.java#L117

At the moment the constant factSinglePeAmplitudeInMv which is used here, is 10.
but we can also convert these integralGains from the file into this number.
The distribution for all pixels looks like this then:
conversion_factors

@jebuss
Copy link
Member

jebuss commented Jan 20, 2017

Good idea, makes sense to me. Let's test it. However the amplitude might only be a 10% effect at max.

@dneise
Copy link
Member Author

dneise commented Jan 20, 2017

Is it a good idea then? Or a waste of time? We have very very limit amout of time only.
The single photon extraction pass should have been started since long.

Question is not whether its nice or not ... surely one can do it and test it ... but is it necessary?
Will people say: "This extraction is invalid", if we do not use these individual gains?

@jebuss
Copy link
Member

jebuss commented Jan 20, 2017

I don't know if it is necessary or not. But I think it would not make the result worse, only better! That's my guess. So put it in, if you agree. If we don't use it, we might misestimate Cherenkov pulses by up to 10%, since we apply a 10mv Amplitude to a 9mv pulse, which is also my guess.

@dneise
Copy link
Member Author

dneise commented Jan 20, 2017

since we apply a 10mv Amplitude to a 9mv pulse, which is also my guess.

Have a look at the lower image:
https://cloud.githubusercontent.com/assets/8200858/22109410/e836dcae-de57-11e6-8661-4cd0b738c706.png

It shows the factors, which would be applied instead of the constant factor 10. ...

@jebuss
Copy link
Member

jebuss commented Jan 20, 2017

Yes that's what I meant.

@jebuss
Copy link
Member

jebuss commented Jan 20, 2017

I was talking about the worst case which is the outlier at 9mv

@jebuss
Copy link
Member

jebuss commented Jan 20, 2017

To shorten the discussion. I think it's a good idea. I think it's worth it. I thinking would improve the result. I think we should go for it.

@dneise
Copy link
Member Author

dneise commented Jan 20, 2017

Adrian agreed with you

@dneise dneise self-assigned this Jan 20, 2017
@relleums relleums self-assigned this Jan 21, 2017
@relleums
Copy link
Member

relleums commented Jan 21, 2017

I introduced a single pulse gain calibration service to avoid code duplication. However, I needed to modify ALL xmls. See PR #207

@maxnoe
Copy link
Member

maxnoe commented Feb 16, 2018

Related to #267

@maxnoe
Copy link
Member

maxnoe commented Mar 15, 2018

The GainService is now in the master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants