Skip to content

TRD: merging TracketCheck and TrackletCountCheck#2201

Merged
Barthelemy merged 5 commits into
AliceO2Group:masterfrom
deependra170598:ModifyTrackletsCountCheck
Mar 28, 2024
Merged

TRD: merging TracketCheck and TrackletCountCheck#2201
Barthelemy merged 5 commits into
AliceO2Group:masterfrom
deependra170598:ModifyTrackletsCountCheck

Conversation

@deependra170598
Copy link
Copy Markdown
Contributor

Hi @martenole !
In this PR , I tried to merge two checks: TrackletCheck and TrackletCountCheck.
Please check if everything seems fine. If everything is fine I will remove commented statements.

One issue I see is that check class can read default parameters written in code
for example:
mStatThresholdPerTrigger = std::stof(mCustomParameters.atOrDefaultValue("StatThresholdPerTrigger", "1000.0", *mActivity));

but can not read default parameter provided with config
for example:
Screenshot 2024-03-22 041100

Comment thread Modules/TRD/src/TrackletCountCheck.cxx Outdated

ILOG(Debug, Devel) << "initializing TrackletCountCheck" << ENDM;

mThresholdMeanHighPerTimeFrame = std::stof(mCustomParameters.atOrDefaultValue("UpperthresholdPerTimeFrame", "600000.0" /*default value*/, *mActivity));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I stumbled upon this as well and don't really like the framework API. Basically you should not use the atOrDefaultValue method, as this will give you always the default value from the code and not from the json unless you have provided a value for the explicit activity type and beamType. Took me also a bit to figure out what to use. Please change all these to use atOptional like below:

Suggested change
mThresholdMeanHighPerTimeFrame = std::stof(mCustomParameters.atOrDefaultValue("UpperthresholdPerTimeFrame", "600000.0" /*default value*/, *mActivity));
mThresholdMeanHighPerTimeFrame = std::stof(mCustomParameters.atOptional("UpperthresholdPerTimeFrame", *mActivity).value_or("6e5"));

I also think that with the scientific notation long numbers are more easy to read..

@deependra170598 deependra170598 marked this pull request as ready for review March 22, 2024 16:18
@deependra170598 deependra170598 marked this pull request as draft March 27, 2024 08:00
@deependra170598 deependra170598 marked this pull request as ready for review March 27, 2024 08:18
Copy link
Copy Markdown
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.

Did not go through everything in detail, but looks good to me

@Barthelemy Barthelemy enabled auto-merge (squash) March 28, 2024 08:03
@Barthelemy Barthelemy merged commit c42f111 into AliceO2Group:master Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants