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 6 commits into
base: main
Choose a base branch
from
Open

Add in quadfitter #220

wants to merge 6 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.

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.

3 participants