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

Add in quadfitter #220

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Add in quadfitter #220

wants to merge 8 commits into from

Conversation

smnaugle
Copy link
Contributor

@smnaugle smnaugle commented Jan 7, 2025

Adds in quadfitter as a rat processor that can be used by analyzers.

TODO: Should add in an experiment agnostic way for users to define what ratdb index to use for quadfitter.

Adding in quadfitter
Copy link
Contributor

@JamesJieranShen JamesJieranShen left a comment

Choose a reason for hiding this comment

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

Hey Sam, thanks for taking the time to do this! I left a couple comments mostly on code structuring, haven't tested it out yet.

@@ -0,0 +1,8 @@
{
name: "FIT_QUAD",
Copy link
Contributor

Choose a reason for hiding this comment

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

This data entry should be placed in FITTER.ratdb, with the index of QUAD, rather than initing its own table.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See my other comment. Happy to debate this.

src/fit/CMakeLists.txt Outdated Show resolved Hide resolved
src/fit/include/RAT/FitQuadProc.hh Outdated Show resolved Hide resolved
src/fit/include/RAT/FitQuadProc.hh Outdated Show resolved Hide resolved
src/fit/src/FitQuadProc.cc Outdated Show resolved Hide resolved
ratdb/FIT_QUAD.ratdb Show resolved Hide resolved
@JamesJieranShen
Copy link
Contributor

TODO: Should add in an experiment agnostic way for users to define what ratdb index to use for quadfitter.

Yes -- I'm working on that;)

@smnaugle
Copy link
Contributor Author

smnaugle commented Jan 8, 2025

Hi @JamesJieranShen, thanks for the comments! I agree with all of them aside from the one about where to put the fitter ratdb table. Are we sure that we want to keep the tables for all fitters in the same file? I feel like that may become a bookkeeping nightmare, in SNO+ we allow each fitter to have it's own ratdb file. Each fitter might have a pretty large number of indices depending on the lifetime of the experiment and the complexity of the fitter.

@JamesJieranShen
Copy link
Contributor

Hi @JamesJieranShen, thanks for the comments! I agree with all of them aside from the one about where to put the fitter ratdb table. Are we sure that we want to keep the tables for all fitters in the same file? I feel like that may become a bookkeeping nightmare, in SNO+ we allow each fitter to have it's own ratdb file. Each fitter might have a pretty large number of indices depending on the lifetime of the experiment and the complexity of the fitter.

Hey @smnaugle, I was referring to the json/ratdb internal data-structure. It should be in the same "table" as the other fitter, i.e. have the same "name", but different "index":

{
"name": "Fitter",
"index": "QUAD",
...
}

Onto splitting the fitters to different files -- I'm ok with putting the fitter ratdb entry as a separate file for book keeping purposes. However, clearly the current purpose of the FITTER.ratdb file is meant to include all fitter parameters. I would suggest changing the file of that name to FITTENSOR.ratdb if you would like to have a separate FITQUAD.ratdb.

Thanks again!

Copy link
Contributor

@JamesJieranShen JamesJieranShen left a comment

Choose a reason for hiding this comment

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

LGTM, on the code front. I will leave to @MarcFBergevin and others to see if there's anything else wrong.

@smnaugle
Copy link
Contributor Author

This PR currently crashes when running quadfitter on data. We should hold off on merging until I find the bug.

@smnaugle
Copy link
Contributor Author

smnaugle commented Jan 14, 2025

OK the previously mentioned crash is now fixed and this PR should be ready to merge in.

One thing to note though, even when I return Processor::Result(FAIL); (line 216 of FitQuadProc.cc), the validposition_quadfitter branch in the output ntuples is true. Is that the intended behavior? If so, how do I make the fit result invalid? I am currently just returning [0,0,0] as the fit result for failed fits.

Edit: Resolved in future commit.

if (pmt->GetDigitizedTime() > 1e6) {
continue;
}
pmtt.push_back(pmt->GetDigitizedTime());
Copy link
Contributor

Choose a reason for hiding this comment

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

We should have a method for how to select which timing to use in the reconstruction algorithms.

@tannerbk
Copy link
Contributor

I think it would make sense to merge #226 first and adapt this PR to use the functionality provided there.

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.

4 participants