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.

Sign up for GitHub

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

Jump to bottom

Adding TRD t0 fitting procedure #11695

Merged
merged 14 commits into from
Aug 7, 2023
Merged

Adding TRD t0 fitting procedure #11695

merged 14 commits into from
Aug 7, 2023

Conversation

luisabergmann
Copy link
Collaborator

The fits are done both for the PH distribution inclusive over all chambers and (if enough statistics are available) for the individual chambers.

added files:

  • TRDPHReaderSpec reads the data produced by the PHData type
  • T0FitSpec steers the procedure
  • T0Fit performs the fits
  • T0FitHistos is a helper class that passes the data from the TRDPHReader to T0Fit

updatet files:

  • the option "t0" is added to the calibration workflow
  • an option to set the average t0 is added to CalT0.h
  • a default value t0 = 1.2 is added to Constants.h
  • a missing library is added to PHData.h
  • the required minimum entries to perform the fits are added to CalibrationParams.h

luisabergmann added a commit to luisabergmann/AliceO2 that referenced this pull request Jul 25, 2023
@luisabergmann
Merge pull request #3 from alibuild/alibot-cleanup-11695
62be1fc 
Please consider the following formatting changes to AliceO2Group#11695
martenole
martenole reviewed Jul 25, 2023
View reviewed changes
Copy link
Contributor

@martenole martenole left a comment

Choose a reason for hiding this comment

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

Hi Luisa, thanks a lot this looks already very good. I only have some minor comments (cleanup, documentation) so we should be able to merge this soon

Detectors/TRD/calibration/src/T0Fit.cxx Outdated Show resolved Hide resolved
Detectors/TRD/workflow/include/TRDWorkflow/T0FitSpec.h Outdated Show resolved Hide resolved
Detectors/TRD/calibration/src/T0Fit.cxx Outdated Show resolved Hide resolved
Detectors/TRD/calibration/src/T0Fit.cxx Outdated Show resolved Hide resolved
Detectors/TRD/calibration/src/T0Fit.cxx Outdated Show resolved Hide resolved
Detectors/TRD/calibration/include/TRDCalibration/T0Fit.h Outdated Show resolved Hide resolved
DataFormats/Detectors/TRD/include/DataFormatsTRD/T0FitHistos.h Outdated Show resolved Hide resolved
DataFormats/Detectors/TRD/include/DataFormatsTRD/T0FitHistos.h Outdated Show resolved Hide resolved
DataFormats/Detectors/TRD/include/DataFormatsTRD/CalT0.h Show resolved Hide resolved
f3sch
f3sch requested changes Aug 1, 2023
View reviewed changes
Copy link
Collaborator

@f3sch f3sch left a comment

Choose a reason for hiding this comment

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

@luisabergmann very nice work, thank you!
I have some very minor suggestions.

DataFormats/Detectors/TRD/src/T0FitHistos.cxx Outdated Show resolved Hide resolved
DataFormats/Detectors/TRD/src/T0FitHistos.cxx Outdated Show resolved Hide resolved
DataFormats/Detectors/TRD/src/T0FitHistos.cxx Show resolved Hide resolved
Detectors/TRD/calibration/src/T0Fit.cxx
Comment on lines +89 to +96
mFitFunctor.lowerBoundFit = 0;
mFitFunctor.upperBoundFit = 5;

double mParamsStart[4];
mParamsStart[0] = 100;
mParamsStart[1] = 500;
mParamsStart[2] = 0.5;
mParamsStart[3] = 0.5;
Copy link
Collaborator

Choose a reason for hiding this comment

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

These magical constants seem all like they should be moved to be configurable parameter.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How would I do that? Do I add lines in T0FitSpec.h like for "sec-per-slot" or "max-delay" ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry to be unclear, I meant to put them in CalibrationParams.h, that way you can control them from the outside later on (if the need ever arises)

Detectors/TRD/calibration/src/T0Fit.cxx Outdated Show resolved Hide resolved
luisabergmann and others added 3 commits August 3, 2023 12:26
@luisabergmann @f3sch
Update DataFormats/Detectors/TRD/src/T0FitHistos.cxx
44942ef 
Co-authored-by: Felix Schlepper <f3sch.git@outlook.com>
@luisabergmann @f3sch
Update DataFormats/Detectors/TRD/src/T0FitHistos.cxx
2382abc 
Co-authored-by: Felix Schlepper <f3sch.git@outlook.com>
@luisabergmann @f3sch
Update Detectors/TRD/calibration/src/T0Fit.cxx
e563022 
Co-authored-by: Felix Schlepper <f3sch.git@outlook.com>
@alibuild
Copy link
Collaborator

alibuild commented Aug 3, 2023

Error while checking build/O2/fullCI for e563022 at 2023-08-03 12:41:

## sw/BUILD/O2-latest/log
/sw/SOURCES/O2/11695-slc8_x86-64/0/DataFormats/Detectors/TRD/src/T0FitHistos.cxx:22:6: error: no declaration matches 'void o2::trd::T0FitHistos::fill(const std::vector<o2::trd::PHData>&)'
ninja: build stopped: subcommand failed.

Full log here.

@f3sch
Copy link
Collaborator

f3sch commented Aug 7, 2023

As a general point, it would be really nice if you could document the procedure and the parameters briefly in the readme :)

martenole
martenole approved these changes Aug 7, 2023
View reviewed changes
Copy link
Contributor

@martenole martenole left a comment

Choose a reason for hiding this comment

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

I think it looks very good as it is now. I am going to merge it and further improvements can of course still be added in followup PRs :)

@martenole martenole merged commit c3d759a into AliceO2Group:dev Aug 7, 2023
6 checks passed
mwinn2 pushed a commit to mwinn2/AliceO2 that referenced this pull request Aug 24, 2023
@luisabergmann @alibuild
@f3sch @mwinn2
Adding TRD t0 fitting procedure (AliceO2Group#11695)
8405d4c 
* Adding TRD t0 fitting procedure

* cleanup/documentation

* Please consider the following formatting changes

* replaced entries-variable with direct instance of calibration parameters

* removed reset() from calibration container

* Please consider the following formatting changes

* Update DataFormats/Detectors/TRD/src/T0FitHistos.cxx

Co-authored-by: Felix Schlepper <f3sch.git@outlook.com>

* Update DataFormats/Detectors/TRD/src/T0FitHistos.cxx

Co-authored-by: Felix Schlepper <f3sch.git@outlook.com>

* Update Detectors/TRD/calibration/src/T0Fit.cxx

Co-authored-by: Felix Schlepper <f3sch.git@outlook.com>

* Updated DataFormats/Detectors/TRD/include/DataFormatsTRD/T0FitHistos.h

---------

Co-authored-by: ALICE Action Bot <alibuild@cern.ch>
Co-authored-by: Felix Schlepper <f3sch.git@outlook.com>
nbize pushed a commit to nbize/AliceO2 that referenced this pull request Aug 31, 2023
@luisabergmann @alibuild
@f3sch @nbize
Adding TRD t0 fitting procedure (AliceO2Group#11695)
b08db8b 
* Adding TRD t0 fitting procedure

* cleanup/documentation

* Please consider the following formatting changes

* replaced entries-variable with direct instance of calibration parameters

* removed reset() from calibration container

* Please consider the following formatting changes

* Update DataFormats/Detectors/TRD/src/T0FitHistos.cxx

Co-authored-by: Felix Schlepper <f3sch.git@outlook.com>

* Update DataFormats/Detectors/TRD/src/T0FitHistos.cxx

Co-authored-by: Felix Schlepper <f3sch.git@outlook.com>

* Update Detectors/TRD/calibration/src/T0Fit.cxx

Co-authored-by: Felix Schlepper <f3sch.git@outlook.com>

* Updated DataFormats/Detectors/TRD/include/DataFormatsTRD/T0FitHistos.h

---------

Co-authored-by: ALICE Action Bot <alibuild@cern.ch>
Co-authored-by: Felix Schlepper <f3sch.git@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@f3sch f3sch f3sch requested changes

@martenole martenole martenole approved these changes

@bazinski bazinski Awaiting requested review from bazinski bazinski is a code owner

@tdietel tdietel Awaiting requested review from tdietel

@pachmay pachmay Awaiting requested review from pachmay

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@luisabergmann @alibuild @f3sch @martenole