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

Common: add generic checker for trending plots #2383

Merged

Conversation

aferrero2707
Copy link
Contributor

@aferrero2707 aferrero2707 commented Aug 12, 2024

The checker is designed to verify that the values of trend graphs are within given minimum and maximum limits.
The limits can be specified as fixed values, or values relative to either the mean or the standard deviation of a given set of graph points.

See https://its.cern.ch/jira/browse/QC-1216 for more details.

The `std::string` type was not correctly handled because the standard
library does not provide an "identity" override for `std::to_string()`
The ultility class provides the following functionalities:
* stores interaction-rate-dependent threshold values
* stores optional axis ranges on which the check should be restricted
* provides the code to retrieve the threshold and range values from the custom parameters
* provides a function to retrieve the optimal threshold values for a given interaction rate
The checker is designed to verify that the values of trend graphs are
within given minimum and maximum limits.
The limits can be specified as fixed values, or values relative to either
the mean or the standard deviation of a given set of graph points.
@aferrero2707 aferrero2707 force-pushed the common-add-generic-trend-checker branch from 9f337bf to 83fdd47 Compare September 20, 2024 13:59
@aferrero2707 aferrero2707 changed the title [WIP] Common: add generic checker for trending plots Common: add generic checker for trending plots Sep 20, 2024
@aferrero2707
Copy link
Contributor Author

aferrero2707 commented Sep 22, 2024

@Barthelemy @knopers8 this PR is rather big, but it does not touch existing code, it only introduces a new checker + some utility class to handle the configuration parameters.
Once it will be possible to retrieve the current interaction rate from CTP, the code is already designed to take advantage of it by specifying rate-dependent checker thresholds in the configuration.

More details are given in the JIRA ticket linked in the description.

@aferrero2707
Copy link
Contributor Author

@Barthelemy would you have the possibility to review this PR in the next days? Apart from the support for rate-dependent checker thresholds (which might only be useful at a later stage), the code could be already used to check the trends of the raw and CTF sizes, for example, and to replace the custom checker for the vertex positions and sigmas.
Thanks!

@Barthelemy
Copy link
Collaborator

@aferrero2707 yes, apologies, this slipped through some hole and disappeared from my radar.

Copy link
Collaborator

@Barthelemy Barthelemy left a comment

Choose a reason for hiding this comment

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

Very good, thanks.

Could you perhaps add a bit of doc ?
I am thinking that we should split the doc further as it becomes very large. We could have a separate page for the "commons".
I'll do it once this is merged.

Modules/Common/src/TrendCheck.cxx Outdated Show resolved Hide resolved
@aferrero2707
Copy link
Contributor Author

@Barthelemy thanks for reviewing!

Could you perhaps add a bit of doc ?

Yes, I was indeed planning to do that, I just wanted to wait for the first feedback in case it would required significant re-work.
What it the appropriate documentation file for putting the documentation for checkers?

@Barthelemy
Copy link
Collaborator

@aferrero2707 You can put it in Advanced.md for now. I will refactor the doc and move all the info about common in a separate file.

@aferrero2707
Copy link
Contributor Author

aferrero2707 commented Oct 19, 2024

@Barthelemy I did the suggested change and added the documentation. Let me know if all is good for you.
Thanks!

Copy link
Collaborator

@Barthelemy Barthelemy left a comment

Choose a reason for hiding this comment

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

Great ! thank you

@Barthelemy Barthelemy enabled auto-merge (squash) October 21, 2024 05:03
@Barthelemy Barthelemy merged commit ae2424b into AliceO2Group:master Oct 21, 2024
7 checks passed
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.

2 participants