star-bnl / star-sw

Core software for STAR experiment
26 stars 63 forks source link

Fix in EmcRawMaker for BEMC data size in StEvent/MuDst #329

Closed starsdong closed 2 years ago

starsdong commented 2 years ago

Please refer to this blog for details [https://drupal.star.bnl.gov/STAR/blog/dongx/BEMC-data-bfc-picoDst-production] Previous related PR #314

genevb commented 2 years ago

I tested this fix on 50 Run 19 AuAu200 events and found the following file sizes for... without this PR: 16239210 Mar 17 15:35 st_physics_20192002_raw_1500018.MuDst.root 4172852 Mar 17 15:35 st_physics_20192002_raw_1500018.picoDst.root with this PR: 12867231 Mar 17 15:35 FIX/st_physics_20192002_raw_1500018.MuDst.root 4172852 Mar 17 15:35 FIX/st_physics_20192002_raw_1500018.picoDst.root

This projects to a ~20% reduction in the total DST volume of the Run 19 AuAu200 production if we can get this fix merged and tagged immediately. (We unfortunately have to restart the Run 19 AuAu200 production as we had a typo in the jobs we started yesterday.) That would be valuable because we're really tight on NFS space for the DST output at the moment.

-Gene

starsdong commented 2 years ago

Hi Gene, THanks. Can you point to me the daq files on disk for this dataset? As you see from my comparison on the blog page, the reduction factor depends on the datasets. I can run a test with previous setup (or may just using an old library) to see the baseline size. This can give us a better understanding whether we have resolved this consistently. Thanks

starsdong commented 2 years ago

Hi Gene and Dmitri, the CI build tests are all OK. I merged the PR. Please feel free to re-tag SL22a. Thanks

plexoos commented 2 years ago

Xin, I am a bit confused by your last comments. Are we ready to declare a new release or you still want to test the code with the changes from #329? Have you performed the same test as Gene reported 6 days ago https://github.com/star-bnl/star-sw/pull/314#issuecomment-1065546654?

starsdong commented 2 years ago

Dmitri, I did several tests with different datasets before (see blog page in the description above). The reduction factor depends on the datasets. I didn't try on the Run19 AuAu200 yet just to understand for this one. But I think the PR itself is ready and can be added to SL22a.

genevb commented 2 years ago

Hi, Xin

On Mar 17, 2022, at 4:08 PM, Xin Dong @.**@.>> wrote:

Can you point to me the daq files on disk for this dataset?

My test job: root4star -b -q -l 'bfc.C(50,"DbV20220317 P2019a StiCA -beamline3D PicoVtxDefault PicoCovMtxWrite BEmcChkStat CorrY -OPr13 -hitfilt","/star/rcf/test/daq/2019/192/st_physics_20192002_raw_1500018.daq")'

-Gene

plexoos commented 2 years ago

Gene, do you plan to do more tests for this fix before it is tagged?

genevb commented 2 years ago

Hi, Dmitri. No, I don't plan to run more tests myself - I am relying on Xin's tests as confirmation that this PR does what it is intended to do. -Gene

On Mar 17, 2022, at 5:33 PM, Dmitri Smirnov @.**@.>> wrote:

Gene, do you plan to do more tests for this fix before it is tagged?

— Reply to this email directly, view it on GitHubhttps://github.com/star-bnl/star-sw/pull/329#issuecomment-1071519719, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AUK2OBICLW75U6L4BMB2PPDVAOQJDANCNFSM5Q5F3IRA. Triage notifications on the go with GitHub Mobile for iOShttps://urldefense.com/v3/__https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675__;!!P4SdNyxKAPE!UV5f-RymRmXSxDk36h5P_ZhHAz4UCkqUnaMjtiI0KIr-aMW0QfU5YOKsujA1$ or Androidhttps://urldefense.com/v3/__https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign*3Dnotification-email*26utm_medium*3Demail*26utm_source*3Dgithub__;JSUlJSU!!P4SdNyxKAPE!UV5f-RymRmXSxDk36h5P_ZhHAz4UCkqUnaMjtiI0KIr-aMW0QfU5YBaJa6hv$. You are receiving this because your review was requested.Message ID: @.***>

starsdong commented 2 years ago

Hi Gene and Dmitri, I added the test for Run19 AuAu200 GeV data provided by Gene and also I added another test with SL21d library production. Please see the update blog page [https://drupal.star.bnl.gov/STAR/blog/dongx/BEMC-data-bfc-picoDst-production] From the comparison, we can see the increase due to the previous commit in MuDst size does have a dependence on different datasets. But I think we can confidently say, the size issue is understood and resolved.

plexoos commented 2 years ago

Okay... Looks like there is no intention to wait for the nightly tests. One more question, hopefully last one, do we want to include the changes from #328 on the SL22a branch? I don't remember if StRefMultCorr is used in production or not.

genevb commented 2 years ago

Hi, Dmitri

On Mar 17, 2022, at 6:36 PM, Dmitri Smirnov @.**@.>> wrote:

Okay... Looks like there is no intention to wait for the nightly tests.

I believe Xin's tests represent several of the nightly tests. Not complete, but the potential gain to moving forward with this now is probably worth the remaining risk.

One more question, hopefully last one, do we want to include the changes from #328https://github.com/star-bnl/star-sw/pull/328 on the SL22a branch? I don't remember if StRefMultCorr is used in production or not.

I do not see the "CentralityMaker" (from PR #328) in the production logs. I don't think it matters: include it or not, whichever is easier. Well, maybe it's better to include it in SL22a_1 so that there exists a tag of it in its current state for anyone using it in analyses.

-Gene

plexoos commented 2 years ago

I believe anyone watching this repo should receive the notification. But just in case, here is the link: https://github.com/star-bnl/star-sw/releases/tag/SL22a_1