Skip to content

PWGHF: Lc task: Remove the redundant Configurable and keep using the Filter - #5232

Merged
vkucera merged 5 commits into
AliceO2Group:masterfrom
zhangbiao-phy:master
Mar 19, 2024
Merged

PWGHF: Lc task: Remove the redundant Configurable and keep using the Filter#5232
vkucera merged 5 commits into
AliceO2Group:masterfrom
zhangbiao-phy:master

Conversation

@zhangbiao-phy

@zhangbiao-phyzhangbiao-phy commented Mar 19, 2024

Copy link
Copy Markdown
Collaborator

mfaggin
mfaggin previously approved these changes Mar 19, 2024
@zhangbiao-phy

Copy link
Copy Markdown
CollaboratorAuthor

Hi @vkucera@DelloStritto, this is the new PR based on the previous comments. I keep using the Filter since there is no need to use Partition in the Lc task for the moment.

@vkuceravkucera changed the title PWGHF: remove the redundant Configurable and keep using the FilterPWGHF: remove the redundant Configurable and keep using the Filter in the Lc taskMar 19, 2024
@vkuceravkucera changed the title PWGHF: remove the redundant Configurable and keep using the Filter in the Lc taskPWGHF: Lc task: Remove the redundant Configurable and keep using the FilterMar 19, 2024
Comment threadPWGHF/D2H/Tasks/taskLc.cxx Outdated
Comment threadPWGHF/D2H/Tasks/taskLc.cxx
zhangbiao-phyand others added 2 commits March 19, 2024 10:07
Co-authored-by: Vít Kučera <vit.kucera@cern.ch>
@vkucera

Copy link
Copy Markdown
Collaborator

Thanks @zhangbiao-phy , did you test the changes locally and can you confirm that you are getting the same results now as before #5040 ?

@alibuild

Copy link
Copy Markdown
Collaborator

Error while checking build/O2Physics/o2 for cade64e at 2024-03-19 11:42:

## sw/BUILD/O2Physics-latest/log
/sw/SOURCES/O2Physics/5232-slc7_x86-64/0/PWGHF/D2H/Tasks/taskLc.cxx:667:21: error: 'selectedLcCandidatesMcMl' was not declared in this scope; did you mean 'selectedLcCandidatesMlMc'?
ninja: build stopped: subcommand failed.

Full log here.

@zhangbiao-phy

Copy link
Copy Markdown
CollaboratorAuthor

Thanks @zhangbiao-phy , did you test the changes locally and can you confirm that you are getting the same results now as before #5040 ?
@vkucera, yes, I have done the test. I have many more candidates from the #5040 w.r.t the keep using the "Filter". Then the candidates look reasonable now. Thanks for spotting this issue!

@vkucera

vkucera commented Mar 19, 2024

Copy link
Copy Markdown
Collaborator

Thanks @zhangbiao-phy , did you test the changes locally and can you confirm that you are getting the same results now as before #5040 ?
@vkucera, yes, I have done the test. I have many more candidates from the #5040 w.r.t the keep using the "Filter". Then the candidates look reasonable now. Thanks for spotting this issue!

Thanks @zhangbiao-phy for checking. However, this is not what I asked. Sorry if I was not clear. I want you to compare the histograms produced with your PR branch and histograms produced with master before #5040 was merged.
We need to validate this fix by confirming that we get the same results as before the bug was introduced.

@zhangbiao-phy

Copy link
Copy Markdown
CollaboratorAuthor

Thanks @zhangbiao-phy , did you test the changes locally and can you confirm that you are getting the same results now as before #5040 ?
@vkucera, yes, I have done the test. I have many more candidates from the #5040 w.r.t the keep using the "Filter". Then the candidates look reasonable now. Thanks for spotting this issue!

Thanks @zhangbiao-phy for checking. However, this is not what I asked. Sorry if I was not clear. I want you to compare the histograms produced with your PR branch and histograms produced with master before #5040 was merged. We need to validate this fix by confirming that we get the same results as before the bug was introduced.

Hi @vkucera, ah, OK, then I finished this check. I get the same output before and after for the case without ML.

@vkucera

Copy link
Copy Markdown
Collaborator

Thanks @zhangbiao-phy , did you test the changes locally and can you confirm that you are getting the same results now as before #5040 ?
@vkucera, yes, I have done the test. I have many more candidates from the #5040 w.r.t the keep using the "Filter". Then the candidates look reasonable now. Thanks for spotting this issue!

Thanks @zhangbiao-phy for checking. However, this is not what I asked. Sorry if I was not clear. I want you to compare the histograms produced with your PR branch and histograms produced with master before #5040 was merged. We need to validate this fix by confirming that we get the same results as before the bug was introduced.

Hi @vkucera, ah, OK, then I finished this check. I get the same output before and after for the case without ML.

Great, thanks @zhangbiao-phy

@vkucera
vkucera enabled auto-merge (squash) March 19, 2024 14:02
@vkucera
vkucera merged commit 3bc6e5c into AliceO2Group:masterMar 19, 2024
christianreckziegel pushed a commit to christianreckziegel/O2Physics that referenced this pull request May 2, 2024
hahassan7 pushed a commit to hahassan7/O2Physics that referenced this pull request May 17, 2024
Sign up for freeto 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

@zhangbiao-phy@vkucera@alibuild@mfaggin