Trg simu - Githubissues

starsdong commented 2 years ago

Updates in StPicoDstMaker to save BEMC data for daq->picoDst production
Adjustments in chain options to allow TriggerSimuMaker in daq->picoDst production
Changes in TriggerSimuMaker for some crashes

starsdong commented 2 years ago

Akio, if possible, could you please take a closer review of the proposed change in StTriggerSimuMaker and StBemcTriggerSimu? I am not an expert in these two. The proposed changes are just hot fixes to avoid crashes. Would appreciate if you can scrutinize the changes. Thanks

zlchang commented 2 years ago

Hi Xin,

I was locked out my BNL accounts recently due to job changes. I don’t have access to RCF either, but I expect it would be back soon. My first suggestion would be add adc2E->SaveAllEvent(kTRUE) in the Init function for StADCtoEMaker similar to what you have done for StTriggerSimuMaker. If you have already done this, it could be a bit tricky to fix the issue. I will think about it.

Zilong

On Feb 25, 2022, at 12:02 AM, Xin Dong @.***> wrote:

@starsdong commented on this pull request.

In StRoot/StTriggerUtilities/Bemc/StBemcTriggerSimu.cxxhttps://github.com/star-bnl/star-sw/pull/313#discussion_r814476760:

@@ -1071,7 +1071,10 @@ void StBemcTriggerSimu::FEEout2009() // emcSim->setCheckStatus(kBarrelEmcTowerId,false); // emcSim->setMakeFullDetector(kBarrelEmcTowerId,true); // emcSim->setDoZeroSuppression(kBarrelEmcTowerId,false);

assert(nhits == kNTowers);

Zilong, thanks. Indeed, I was worried about this change. The reason was that the code crashes at this line, see the log file /star/u/dongx/pwg/Git/20220224/test/test.log or under that directory, run "bfc.C(10,"pp2022,StiCA,BEmcChkStat,epdhit,-hitfilt,-evout,-fcsCluster,-fcsPoint","/gpfs01/star/daq/2022/032/23032031/st_physics_adc_23032031_raw_6500079.daq")" As you mentioned, this may be related to the pedestal DB entry that is related to what Zhenyu is trying to fix. If you get a chance, maybe you have a better solution. Thanks

— Reply to this email directly, view it on GitHubhttps://github.com/star-bnl/star-sw/pull/313#discussion_r814476760, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AGH7F3MYL446T7WQODGI7NTU44EN5ANCNFSM5PIU3CZA. You are receiving this because your review was requested.Message ID: @.***>

genevb commented 2 years ago

I wrote a note for the proposed changes to BigFullChain.h several hours ago, but the note remains in a "Pending" state for a reason that is unknown to me. So I'll copy it here as a general comment in the conversation for this PR at the risk of it being redundant....

It appears that this proposal is now to include StEmcADCtoEMaker in the reconstruction chain. I'm doubtful anyone is maintaining that code, though my understanding is that it may be used for the JetTree productions. I guess the CODEOWNERS file indicates that @kkauder and @rkunnawa might be maintainers through their association with all StRoot/StEmc* directories, but it would be good to have that confirmed.

Additionally, as far as I understand, the purpose of that code has been duplicated through StEmcRawMaker [and its calls to StEmcUtil/database/StBemcTables::getCalib()], which itself does usually run in BFC chains already. So it would be helpful to understand why we need to have StEmcADCtoEMaker as well.

-Gene

starsdong commented 2 years ago

Zilong, thanks for the suggestion. I can give a test to check this out. I saw this comment line, but was not sure if SaveAllEvent turned on, does it add a large use of memory or any other concern?

starsdong commented 2 years ago

Gene, my understanding is that StEmcAdcToEMaker is needed to obtain the calibrated energy information for the towers. This was also suggested by Zilong. I missed this in the previous PR. I didn't know much about the StEmcRawMaker. EmcRawMaker was always in the chain, but I think the EMC energy information is missing in picoDst as the original issue. I should let Zilong ( @zlchang ) or Akio ( @akioogawa ) to comment.

Regarding the maintenance, I agree we should confirm this if we go with including it in the production chain. I will confirm with @kkauder and @rkunnawa .

klendathu2k commented 2 years ago

Hi Xin, Gene,

On (near) line 1644 of BigFullChain.h--

{"emcAtoE" ,"bemcA2E","" ,"db","StEmcADCtoEMaker","StEmcADCtoEMaker" , "B-EMC ADC to E converter OBSOLETE for data in Run 9 or later",kFALSE},

Comment field suggests that this maker should not be used for any data following run 9. Until an expert confirms that the maker is valid, I would take any results from it as suspect.

Cheers, Jason

On 2022-02-25 10:55, Xin Dong wrote:

Gene, my understanding is that StEmcAdcToEMaker is needed to obtain the calibrated energy information for the towers. This was also suggested by Zilong. I missed this in the previous PR. I didn't know much about the StEmcRawMaker. EmcRawMaker was always in the chain, but I think the EMC energy information is missing in picoDst as the original issue. I should let Zilong ( @zlchang [1] ) or Akio ( @akioogawa [2] ) to comment.

Regarding the maintenance, I agree we should confirm this if we go with including it in the production chain. I will confirm with @kkauder [3] and @rkunnawa [4] .

-- Reply to this email directly, view it on GitHub [5], or unsubscribe [6]. Triage notifications on the go with GitHub Mobile for iOS [7] or Android [8]. You are receiving this because your review was requested.Message ID: @.***>

Links:

[1] https://github.com/zlchang [2] https://github.com/akioogawa [3] https://github.com/kkauder [4] https://github.com/rkunnawa [5] https://github.com/star-bnl/star-sw/pull/313#issuecomment-1050974841 [6] https://github.com/notifications/unsubscribe-auth/ANL4LVBQFYJ3KB4CY5RRIG3U46RAFANCNFSM5PIU3CZA [7] https://urldefense.com/v3/__https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675__;!!P4SdNyxKAPE!ShWDAphrvvVxvzeTNsVCBwv0_vo4mQd0nEQVfERq3h3mp684JEsuQrnfVcqnCfo$ [8] https://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!ShWDAphrvvVxvzeTNsVCBwv0_vo4mQd0nEQVfERq3h3mp684JEsuQrnfSpcRgco$

starsdong commented 2 years ago

Jason, good point. But as Gene mentioned, this maker has been used by ColdQCD group for the JetTree production. In the meantime, our MuDst->picoDst conversion uses genDst.C [https://github.com/star-bnl/star-sw/blob/main/StRoot/macros/mudst/genDst.C] which includes EmcADCtoEMaker in the production. The issue is now also mixed, but I do agree that we should clear out the path both for the current solution and future maintenance. For everyone on this thread, I am thinking to have a dedicate meeting to discuss this issue including SC management + EMC subsystem coordinators + PWG conveners to define a path forward. I will send out a separate email then.

zlchang commented 2 years ago

Hi Xin,

Maybe try not to include “emcAtoE” in the chain option, and see if you see tower energy in the StEvent. If so, we can remove the option, otherwise we can think about ways to fix this.

Zilong

On Feb 25, 2022, at 11:45 AM, Xin Dong @.***> wrote:

Jason, good point. But as Gene mentioned, this maker has been used by ColdQCD group for the JetTree production. In the meantime, our MuDst->picoDst conversion uses genDst.C [https://github.com/star-bnl/star-sw/blob/main/StRoot/macros/mudst/genDst.C] which includes EmcADCtoEMaker in the production. The issue is now also mixed, but I do agree that we should clear out the path both for the current solution and future maintenance. For everyone on this thread, I am thinking to have a dedicate meeting to discuss this issue including SC management + EMC subsystem coordinators + PWG conveners to define a path forward. I will send out a separate email then.

— Reply to this email directly, view it on GitHubhttps://github.com/star-bnl/star-sw/pull/313#issuecomment-1051016511, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AGH7F3OUX3TIOOH62WYD5F3U46W37ANCNFSM5PIU3CZA. You are receiving this because you were mentioned.Message ID: @.***>

akioogawa commented 2 years ago

Hi Xin, I'm not really an expert on B/EEMC trigger, nor B/EEMC offline code (last time I did something was like ~25years ago). I'll try to read through the code but will not be quick...

genevb commented 2 years ago

On Feb 25, 2022, at 10:55 AM, Xin Dong @.**@.>> wrote:

I didn't know much about the StEmcRawMaker. EmcRawMaker was always in the chain, but I think the EMC energy information is missing in picoDst as the original issue. I should let Zilong ( @zlchanghttps://github.com/zlchang ) or Akio ( @akioogawahttps://github.com/akioogawa ) to comment.

Offline QA has non-empty plots with BEMC (BTOW) energy distributions (this has been working for many, many years). Just a few weeks ago I traced the energy values back to being filled via StEmcRawMaker. So I have some confidence that StEmcRawMaker is performing the task. If someone wants to know which functions do the work, I can forward to them the information I have.

-Gene

starsdong commented 2 years ago

Thank you, Gene. I confirmed indeed the energy information can be retrieved from EmcRawMaker. The inclusion of emcADCtoEMaker was due to the TriggerSimuMaker requires it. I had a discussion yesterday with Zilong on this and also the earlier issue on the BTOW nhits assertion error. We think we have fixes for both now and are doing some further tests with various datasets. I am going to close this PR. Will follow along with a different PR.

star-bnl / star-sw

Trg simu #313

Links: