-
Notifications
You must be signed in to change notification settings - Fork 4
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
Fixing problems with TRestDetectorSignal fitting methods #119
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
I think the problem was in this line The bin limits for the histogram were fixed. This works well with signals with 20 ns of sampling time (see figure) but not with signals of 40 ns of sampling time, for example.
What's new:
|
for more information, see https://pre-commit.ci
energy = gaus->GetParameter(0); | ||
time = gaus->GetParameter(1); | ||
energy = gaus.GetParameter(0); | ||
time = gaus.GetParameter(1); | ||
} else { | ||
// the fit failed, return -1 to indicate failure | ||
energy = -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can take this opportunity to rethink if we want to return energy -1
or do something else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made #120 to address this.
Long time ago I did a pull request to add generic methods and this issue was addressed here https://github.com/rest-for-physics/framework/blob/48c4faed4b4942a1fa48b2b1b34acd9432ae950f/source/framework/analysis/src/TRestPulseShapeAnalysis.cxx#L396. There is no need to create a Pull request is here rest-for-physics/framework#379 |
for more information, see https://pre-commit.ci
…ib into mariajmz_fits
for more information, see https://pre-commit.ci
src/TRestDetectorSignal.cxx
Outdated
@@ -424,23 +435,22 @@ TRestDetectorSignal::GetMaxAget() // returns a 2vector with the time of the pea | |||
Double_t lowerLimit = maxRawTime - 0.2; // us | |||
Double_t upperLimit = maxRawTime + 0.7; // us |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't changed this limits (also in the landau fit). Maybe we should adjust them too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think we should. The new dynamic time window should work better in most cases than what we have now.
src/TRestDetectorSignal.cxx
Outdated
} | ||
|
||
TF1 gaus("gaus", "gaus", lowerLimit, upperLimit); | ||
TH1F h1("h1", "h1", GetNumberOfPoints(), GetTime(0), GetTime(GetNumberOfPoints() - 1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are maintaining the fit with the signal stored as histogram but as Juanan commented, it is not necessary as we can get the signal as TGraph with TRestDetectorSignal::GetGraph(Int_t color). What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think TGraph
is a better choice but on practice I don't think it makes a difference since the histogram has one bin per signal time bin, so the information content of both representations is the same.
You can change them to use TGraph
if you want but I would do it in a different PR (unless it's trivial to do so).
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
DON'T MERGE: This is a work in progress
@JPorron and me detected a problem with the fitting methods: the limits for the fits are hardcoded and are only valid for a sampling time of 20 ns.