-
Notifications
You must be signed in to change notification settings - Fork 445
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
GLOQC: Reduce memory footprint #12871
Conversation
f3sch
commented
Mar 15, 2024
•
edited
Loading
edited
- stop creation of MC histos in Data
REQUEST FOR PRODUCTION RELEASES:
This will add The following labels are available |
Changing to ready to test CI. |
TH1D* getHisto1OverPtPhysPrimNum(matchType m) const { return m1OverPtPhysPrimNum[m]; } | ||
TH1D* getHisto1OverPtPhysPrimDen(matchType m) const { return m1OverPtPhysPrimDen[m]; } | ||
TEfficiency* getFractionITSTPCmatchPhysPrim1OverPt(matchType m) const { return mFractionITSTPCmatchPhysPrim1OverPt[m]; } | ||
/* do we need these? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the problem in keeping them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem at all just wondering if this is deadcode or if there is use case? Anyways I just commented them out to test if something will fail.
Error while checking build/O2/fullCI for e6a4c90 at 2024-03-21 03:34:
Full log here. |
Hello, According to https://docs.google.com/spreadsheets/d/1sXokWFLjyceNhTlTcKL_-CczFZ6Vy2IMeCUJ2ujsTWs/edit#gid=0, the gain will anyway be minimal. @f3sch , why don't we keep all TH*D, and change the rest in this PR? Chiara |
220a4bb
to
40e7d6b
Compare
Error while checking build/O2/fullCI for 40e7d6b at 2024-03-22 17:28:
Full log here. |
- stop creation of MC histos in Data Signed-off-by: Felix Schlepper <felix.schlepper@cern.ch>
Hi @chiarazampolli, yes I did revert this part. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, looks fine, the failure is unrelated.
Eventually, I would substitute all pT axes by 1/pT and use less bins, we still have too many 2D histos with 100x200 bins.
@chiarazampolli I did not understand your reference on https://docs.google.com/spreadsheets/d/1sXokWFLjyceNhTlTcKL_-CczFZ6Vy2IMeCUJ2ujsTWs/edit#gid=0 concerning THD vs THF effect: there should be 50% difference for TH1*, and factor 2 for TH2*, so in latter case the double version should be avoided (as it is in this code).
Hello @shahor02 , We also have QC vs 1/pt already, but not 2D. For the THD vs THF, Felix had changed from THD to THF in this PR. We then were not sure since we might hit the limit of precision of THF... So we preferred to keep THD, considering also that the gain in memory did not look so big according to Felix's expectation (written in the spreadsheet). Does this clarify? cheers, |
this was actually my question, in the spreadsheet I did not see anything about THxD vs THxF... |