Skip to content

PWGHF: Add ThnSparse of the ML scores vs Vars to the Lc task#5040

Merged
fgrosa merged 10 commits intoAliceO2Group:masterfrom
zhangbiao-phy:master
Mar 8, 2024
Merged

PWGHF: Add ThnSparse of the ML scores vs Vars to the Lc task#5040
fgrosa merged 10 commits intoAliceO2Group:masterfrom
zhangbiao-phy:master

Conversation

@zhangbiao-phy
Copy link
Collaborator

No description provided.

@zhangbiao-phy zhangbiao-phy changed the title Add ThnSparse of the ML scores vs Vars to the Lc task PWGHF: Add ThnSparse of the ML scores vs Vars to the Lc task Mar 7, 2024
Copy link
Collaborator

@fgrosa fgrosa left a comment

Choose a reason for hiding this comment

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

Hi @zhangbiao-phy thanks a lot! I have few comments (the most important about the maximum number of axes), see below

@zhangbiao-phy
Copy link
Collaborator Author

Hi @zhangbiao-phy thanks a lot! I have few comments (the most important about the maximum number of axes), see below

Hi @fgrosa, Thanks for the review! Very helpful! I commit the changes based on your suggestion and comments.

Copy link
Collaborator

@fgrosa fgrosa left a comment

Choose a reason for hiding this comment

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

Hi @zhangbiao-phy thanks for implementing my comments! I have a couple of additional suggestions, then it's good for me.

@alibuild
Copy link
Collaborator

alibuild commented Mar 8, 2024

Error while checking build/O2Physics/o2 for f938929 at 2024-03-08 12:40:

## sw/BUILD/O2Physics-latest/log
/sw/SOURCES/O2Physics/5040-slc7_x86-64/0/PWGHF/D2H/Tasks/taskLc.cxx:52:92: error: redeclaration of 'o2::framework::ConfigurableAxis HfTaskLc::thnConfigAxisBdtScoreLc'
/sw/SOURCES/O2Physics/5040-slc7_x86-64/0/PWGHF/D2H/Tasks/taskLc.cxx:278:43: error: 'thnConfigAxisBdtScoreBkg' was not declared in this scope; did you mean 'thnConfigAxisBdtScoreLc'?
/sw/SOURCES/O2Physics/5040-slc7_x86-64/0/PWGHF/D2H/Tasks/taskLc.cxx:278:89: error: no matching function for call to 'o2::framework::AxisSpec::AxisSpec(<brace-enclosed initializer list>)'
/sw/SOURCES/O2Physics/5040-slc7_x86-64/0/PWGHF/D2H/Tasks/taskLc.cxx:279:46: error: 'thnConfigAxisBdtScoreSignal' was not declared in this scope; did you mean 'thnConfigAxisBdtScoreLc'?
/sw/SOURCES/O2Physics/5040-slc7_x86-64/0/PWGHF/D2H/Tasks/taskLc.cxx:279:98: error: no matching function for call to 'o2::framework::AxisSpec::AxisSpec(<brace-enclosed initializer list>)'
/sw/SOURCES/O2Physics/5040-slc7_x86-64/0/PWGHF/D2H/Tasks/taskLc.cxx:280:105: error: no matching function for call to 'o2::framework::AxisSpec::AxisSpec(<brace-enclosed initializer list>)'
ninja: build stopped: subcommand failed.

Full log here.

zhangbiao-phy and others added 4 commits March 8, 2024 13:41
Co-authored-by: Vít Kučera <vit.kucera@cern.ch>
Co-authored-by: Vít Kučera <vit.kucera@cern.ch>
@fgrosa fgrosa merged commit 4ca0017 into AliceO2Group:master Mar 8, 2024
@vkucera
Copy link
Collaborator

vkucera commented Mar 18, 2024

@zhangbiao-phy , your changes replaced the filter with partitions which broke the candidate grouping by collision. As a result, all candidates are now processed multiple times, once for each collision. To fix the grouping, please add slicing per collision index.
Also, the added configurable applyMl is misleading, as it only activates histograms filling and does not apply any ML selection, which is only done in the selector.
Please, always test your changes before making a PR.

@vkucera
Copy link
Collaborator

vkucera commented Mar 18, 2024

Tagging @DelloStritto to keep him in the loop.

@zhangbiao-phy
Copy link
Collaborator Author

@zhangbiao-phy , your changes replaced the filter with partitions which broke the candidate grouping by collision. As a result, all candidates are now processed multiple times, once for each collision. To fix the grouping, please add slicing per collision index. Also, the added configurable applyMl is misleading, as it only activates histograms filling and does not apply any ML selection, which is only done in the selector. Please, always test your changes before making a PR.

Hi @vkucera, Thanks for spotting this! It's a mistake. actually, I have tested it on my laptop. I will make a PR soon.

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.

4 participants