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.

Sign up for GitHub

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

Jump to bottom

[PWGCF] Update lambdaR2Correlation.cxx #8779

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[PWGCF] Update lambdaR2Correlation.cxx #8779

wants to merge 3 commits into from
+519 −652

Conversation

yashpatley
Copy link
Contributor

  1. Modified the lambda selection methods
  2. Added correction factor histograms from ccdb

@yashpatley
[PWGCF] Update lambdaR2Correlation.cxx
2aa2f68 
1. Modified the lambda selection methods
2. Added correction factor histograms from ccdb
@github-actions github-actions bot added the pwgcf label Dec 2, 2024
@alibuild
Copy link
Collaborator

alibuild commented Dec 2, 2024

Error while checking build/O2Physics/o2 for a77aaad at 2024-12-02 19:14:

## sw/BUILD/O2Physics-latest/log
/sw/SOURCES/O2Physics/8779-slc9_x86-64/0/PWGCF/TwoParticleCorrelations/Tasks/lambdaR2Correlation.cxx:556:44: error: expected '(' before 'obj'
/sw/SOURCES/O2Physics/8779-slc9_x86-64/0/PWGCF/TwoParticleCorrelations/Tasks/lambdaR2Correlation.cxx:556:56: error: expected ')' before ';' token
/sw/SOURCES/O2Physics/8779-slc9_x86-64/0/PWGCF/TwoParticleCorrelations/Tasks/lambdaR2Correlation.cxx:560:44: error: expected '(' before 'obj'
/sw/SOURCES/O2Physics/8779-slc9_x86-64/0/PWGCF/TwoParticleCorrelations/Tasks/lambdaR2Correlation.cxx:560:56: error: expected ')' before ';' token
/sw/SOURCES/O2Physics/8779-slc9_x86-64/0/PWGCF/TwoParticleCorrelations/Tasks/lambdaR2Correlation.cxx:565:44: error: expected '(' before 'obj'
/sw/SOURCES/O2Physics/8779-slc9_x86-64/0/PWGCF/TwoParticleCorrelations/Tasks/lambdaR2Correlation.cxx:565:56: error: expected ')' before ';' token
ninja: build stopped: subcommand failed.

Full log here.

victor-gonzalez
victor-gonzalez requested changes Dec 2, 2024
View reviewed changes
Copy link
Collaborator

@victor-gonzalez victor-gonzalez left a comment

Choose a reason for hiding this comment

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

Please, have a look at my comment
Functionality should be pushed once it has been tested locally

PWGCF/TwoParticleCorrelations/Tasks/lambdaR2Correlation.cxx
int vz_bin = hist->GetZaxis()->FindBin(col.posZ());
return hist->GetBinContent(pt_bin, rap_bin, vz_bin);
} else {
return 1.;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please, run it locally before pushing the functionality
There are histograms that are cloned on a per V0 basis and that are left there, allocated, which creates memory leaks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dear Victor, I will keep this in mind. I missed compiling the task after the last change, it will not happen again.
I understand the thing about memory leak. I will rectify it in next PR. I am not going to use CCDB object this time because I want to get the correction factors with this task. I added the ccdb method for future purposes. I will keep the memory leak thing in mind and thank you for your suggestion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is not only a matter of compiling
You should not use hyperloop for testing your functionality
Once the functionality is tested locally and confirmed that works then it can be pushed to the central repo for having it available on hyperloop

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay Victor, I will keep this in mind.

victor-gonzalez
victor-gonzalez approved these changes Dec 3, 2024
View reviewed changes
Copy link
Collaborator

@victor-gonzalez victor-gonzalez left a comment

Choose a reason for hiding this comment

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

Have in mind that all histograms are derived from TH1 and that it has a method for knowing the number of dimensions one histogram actually has

@victor-gonzalez victor-gonzalez enabled auto-merge (squash) December 3, 2024 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@victor-gonzalez victor-gonzalez victor-gonzalez approved these changes

@alibuild alibuild Awaiting requested review from alibuild alibuild is a code owner

@saganatt saganatt Awaiting requested review from saganatt saganatt is a code owner

@zchochul zchochul Awaiting requested review from zchochul zchochul is a code owner

@lgraczykCern lgraczykCern Awaiting requested review from lgraczykCern lgraczykCern is a code owner

@prchakra prchakra Awaiting requested review from prchakra prchakra is a code owner

@lauraser lauraser Awaiting requested review from lauraser lauraser is a code owner

@ariedel-cern ariedel-cern Awaiting requested review from ariedel-cern ariedel-cern is a code owner

@otonvd otonvd Awaiting requested review from otonvd otonvd is a code owner

@shouqiye shouqiye Awaiting requested review from shouqiye shouqiye is a code owner

Assignees
No one assigned
Labels
pwgcf
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yashpatley @alibuild @victor-gonzalez