Skip to content

margins for threshold tuning#646

Merged
benedikt-voelkel merged 2 commits into
AliceO2Group:masterfrom
DanielSamitz:master
Sep 27, 2022
Merged

margins for threshold tuning#646
benedikt-voelkel merged 2 commits into
AliceO2Group:masterfrom
DanielSamitz:master

Conversation

@DanielSamitz

Copy link
Copy Markdown
Contributor

No description provided.

@benedikt-voelkel benedikt-voelkel left a comment

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.

You are computing the new threshold here already which will then be passed to the macro. As far as I can see, all margins and this file are passed to the macro where then again the margin is multiplied. That seems a duplication. In fact it should be enough to do the computation here and nothing ales needs to be passed to the macro.

if t["test_name"] not in REL_VAL_TEST_NAMES_MAP:
continue
ind = REL_VAL_TEST_NAMES_MAP[t['test_name']]
f.write(f"{histo_name},{t['test_name']},{t['value']*threshold_margins[ind]}\n")

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.

You are computing the new threshold here already which will then be passed to the macro. As far as I can see, all margins and this file are passed to the macro where then again the margin is multiplied. That seems a duplication. In fact it should be enough to do the computation here and nothing ales needs to be passed to the macro.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You are right. I missed that the root macro is not actually reading from the .json file but from use_thresholds.dat produced by the python script

@benedikt-voelkel
benedikt-voelkel merged commit 316a5ce into AliceO2Group:master Sep 27, 2022
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.

2 participants