update loadParams(const int runNumber) and fix the index issue in StRoot/StBTofUtil/StBTofSimResParams.h #65

Morning-Ye · 2021-07-21T17:27:45Z

changed the void loadParams(const int date = 20160913, const int time = 175725, const char* Default_time = "2016-09-13 17:57:25")
to void loadParams(const int runNumber = 20076002)
and read the BTOF cell resolution table based on the runnumber

…5725, const char* Default_time ) to loadParm(const int runnumber)

Morning-Ye · 2021-07-26T14:10:27Z

Gene mentioned that the 19.6 GeV data production will start soon, shall we approve the merger?

genevb · 2021-07-26T19:48:06Z

Gene mentioned that the 19.6 GeV data production will start soon, shall we approve the merger?

Are you hoping to have this included in SL21c???

Morning-Ye · 2021-07-26T19:56:29Z

Hi Gene, Yes, these updates are done for improving the Startless mode BTOF T0s determination as well as including the nSigmaBTOF values. Thanks, Zaochen

…

On Mon, Jul 26, 2021 at 2:48 PM Gene Van Buren ***@***.***> wrote: Gene mentioned that the 19.6 GeV data production will start soon, shall we approve the merger? Are you hoping to have this included in SL21c??? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#65 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADSL4INSMMF5CZF6QQVFGGLTZW3YFANCNFSM5AYMYTKQ> .

genevb · 2021-07-26T20:02:04Z

I see.... So we'll need SL21c tagged and rebuilt yet again.... -Gene

plexoos · 2021-07-26T21:02:02Z

It is so hard to see what was really changed in the code by looking at the diff. There are so many changes in the white space and additional print statements intertwined with presumably real changes... Hopefully, it will make more sense to the maintainers who will approve soon.

veprbl · 2021-07-27T01:35:12Z

I second @plexoos . The change looks fine, but please don't reformat with tabs.

Morning-Ye · 2021-07-27T02:06:54Z

I second @plexoos . The change looks fine, but please don't reformat with tabs.

Hi Dmitry, Thank you very much, I will never do this again!

genevb · 2021-07-27T15:17:14Z

(EDIT: I see from Zaochen's earlier reply to me that this is needed not just for simulation, but for production as well, reinforcing the point that this needs to be resolved now, so waiting to put these into an SL21c_embed is not an option anyhow.)

Given that embedding for BES-II will likely begin before the production completes, and that we cannot update the library (SL21c) mid-production, we have to wait for this code change to be added before we start the production (unless we create another library, SL21c_embed, though we would preferably not do that). Please understand that there is high pressure to get this production started, so please wrap up this code change (and any others?) that are needed for BES-II with urgency.

starsdong · 2021-07-27T21:35:46Z

Frank and Daniel, would it be possible for you to take a review about this change and approve at your earliest convenience?
From my side, the change looks OK. I hope Zaochen has tested this with the Run19 data production?
One potential concern is that the initialization date is always 0101 (January 1st in each calendar year). It should be OK for the BES-II datasets (Run19, 20, 21 etc.). But for some year that starts before Jan. 1st. (e.g. the upcoming 2022 run), one will need to take care of these runs happening before Jan. 1st.

Morning-Ye · 2021-07-27T21:52:54Z

Hi Xin, For the changes I made, I have tested with the Run19 daq files, it works fine. For the initialization date, as you already pointed out, this should be good for the BES-II. I originally planned to set the timestamp as the input parameter to grep the tables from DB, however, I noticed that it will need more changes in the StBTofCalibrationMaker Init() function. So I did not do it that way. Do we want to make more changes on it now or leave it as the current version and take care in future data production? Thanks, Zaochen

…

On Tue, Jul 27, 2021 at 4:35 PM Xin Dong ***@***.***> wrote: Frank and Daniel, would it be possible for you to take a review about this change and approve at your earliest convenience? From my side, the change looks OK. I hope Zaochen has tested this with the Run19 data production? One potential concern is that the initialization date is always 0101 (January 1st in each calendar year). It should be OK for the BES-II datasets (Run19, 20, 21 etc.). But for some year that starts before Jan. 1st. (e.g. the upcoming 2022 run), one will need to take care of these runs happening before Jan. 1st. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#65 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADSL4IP3DHG3GU3N3YEZ5N3TZ4RD3ANCNFSM5AYMYTKQ> .

starsdong · 2021-07-27T21:55:33Z

Zaochen, Glad that you have thought about this. Indeed, that would be the best and clean solution.
One more question, do we need to worry about the backward compatibility of this commit?

Morning-Ye · 2021-07-27T22:15:51Z

Hi Xin, there is only one set of resolution table in the DB, the previous uploaded table with messed index were overwritten with the new one (2014-2017). However I want to mention it again here, the new BTOF resolution table is obtained based on the Run18 Isobar 200 GeV calibrations. For those years earlier than 2018, the resolution tables may need to be restudied based on Run14 or Run16 data. These resolution tables are used for nSigmaBTOF calculations, actually if people don't use this variable for their analysis, there should be no problem. As the tables were there, thus the backward compatibility should be ok. However, I am not sure whether Bassam's changes could be backward compatible?

…

On Tue, Jul 27, 2021 at 4:55 PM Xin Dong ***@***.***> wrote: Zaochen, Glad that you have thought about this. Indeed, that would be the best and clean solution. One more question, do we need to worry about the backward compatibility of this commit? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#65 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADSL4IKIKM73KD5HYFXAXZ3TZ4TOBANCNFSM5AYMYTKQ> .

genevb · 2021-07-28T01:39:04Z

On this point... On Jul 27, 2021, at 5:35 PM, Xin Dong ***@***.******@***.***>> wrote: One potential concern is that the initialization date is always 0101 (January 1st in each calendar year). It should be OK for the BES-II datasets (Run19, 20, 21 etc.). But for some year that starts before Jan. 1st. (e.g. the upcoming 2022 run), one will need to take care of these runs happening before Jan. 1st. ...the safe bet is October 1st of the prior calendar year, which is the start of the fiscal year.

…

-Gene

Morning-Ye · 2021-07-28T02:18:09Z

Hi Gene, Good suggestion, so the easy solution will be change "*const int date = yearNumber*1000*10 + 101*;" to "*const int date = (yearNumber-1)*1000*10 + 1001;*" If there are no obligations, I will make this change tomorrow. Thanks, Zaochen On Tue, Jul 27, 2021 at 8:39 PM Gene Van Buren ***@***.***> wrote:

…

On this point... On Jul 27, 2021, at 5:35 PM, Xin Dong ***@***.******@***.***>> wrote: One potential concern is that the initialization date is always 0101 (January 1st in each calendar year). It should be OK for the BES-II datasets (Run19, 20, 21 etc.). But for some year that starts before Jan. 1st. (e.g. the upcoming 2022 run), one will need to take care of these runs happening before Jan. 1st. ...the safe bet is October 1st of the prior calendar year, which is the start of the fiscal year. -Gene — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#65 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADSL4IOMMSOHEJEU3DZKXVTTZ5NUFANCNFSM5AYMYTKQ> .

…ct table

starsdong · 2021-07-28T15:33:21Z

Zaochen, thanks. The new commit looks good to me. If you can do a quick test with the 19.6 GeV daq file to be sure something goes wrong with this last commit. Thanks

starsdong

Thanks, Zaochen. The new commits look OK to me.

Morning-Ye · 2021-07-28T19:41:31Z

I am thinking maybe I should let Bassam or someone else submit the pull request, then I can also approve it as a code reviewer. Since Frank and Daniel are not available.

…

On Wed, Jul 28, 2021 at 2:25 PM Xin Dong ***@***.***> wrote: ***@***.**** approved this pull request. Thanks, Zaochen. The new commits look OK to me. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#65 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADSL4IPI5AA3UYZOGNFO6D3T2BKRFANCNFSM5AYMYTKQ> .

starsdong · 2021-07-28T21:18:10Z

Dmitri, I am not sure if Frank or Daniel is immediately available. Is it OK to add another reviewer here for approval so it can get merged later?

plexoos · 2021-07-28T21:23:46Z

Ok, we can merge with the available feedback.
Can Zaochen provide here at least some instruction how he tested the change?

StRoot/StBTofUtil/StBTofSimResParams.h

Morning-Ye · 2021-07-28T22:18:14Z

I ran root4star -b -q -l 'bfc.C ( 1, 10, "DbV20210423 P2019a StiCA -beamline3D btof BEmcChkStat CorrY -OPr13 EbyET0 evout PicoVtxVpdOrDefault PicoCovMtxWrite -hitfilt btofCalib vpdCalib pppAMode", "/star/u/bassam/forGrigory/scratch/daqFor2019AuAu19p6/st_physics_adc_20076002_raw_0000004.daq" )' > & ! log_st_physics_adc_20076002_raw_0000004 & and then look at the print out table values (in the i,j loop, currently removed the cout lines) to what I uploaded to DB, and found they are the same.

…

On Wed, Jul 28, 2021 at 4:23 PM Dmitri Smirnov ***@***.***> wrote: Ok, we can merge with the available feedback. Can Zaochen provide here at least some instruction how he tested the change? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#65 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADSL4IKPNYVEEOJGXTY7P3TT2BYO3ANCNFSM5AYMYTKQ> .

plexoos · 2021-07-28T23:35:01Z

StRoot/StBTofUtil/StBTofSimResParams.h


-		TDataSet *DB = GetDataBase("Calibrations/tof/tofSimResParams");
+		//! Loads BTOF Sim Params from database
+		//void loadParams(const int date = 20160913, const int time = 175725, const char* Default_time = "2016-09-13 17:57:25")


Suggested change

//void loadParams(const int date = 20160913, const int time = 175725, const char* Default_time = "2016-09-13 17:57:25")

plexoos · 2021-07-28T23:35:14Z

StRoot/StBTofUtil/StBTofSimResParams.h

-      mAverageTimeResTof=0;
-			for ( int i = 0; i < 120; i++ ){ //  nTrays
-				for ( int j = 0; j < 192; j++ ){
-					size_t index = i * 120 + j;


Do I understand correctly that the main objective of this PR was to fix the problem with this index?

-size_t index = i * 120 + j; +size_t index = i * 192 + j;

If so, can it be mentioned in the PR title? The current title is not very informative. It will become a log message when we squash and merge

There are two main changes: 1) fix this index issue; 2) load the table according to the runnumber. Both of these changes are within the function loadParmas().

* change void loadParams(const int date = 20160913, const int time = 175725, const char* Default_time ) to loadParm(const int runnumber) * change 120, 192 to mNTray mNCellsPerTray * loadParams(runnumber) * Gene suggested use Previous year Oct 1st to be safe to grep the correct table * modify double timeres_tof() itray-1, imodule-1, icell-1 * add call writePPPAHistogram from Bassam * remove those tem debug lines Co-authored-by: Zaochen Ye <zye20@rcas6015.rcf.bnl.gov>

…-bnl#65) * change void loadParams(const int date = 20160913, const int time = 175725, const char* Default_time ) to loadParm(const int runnumber) * change 120, 192 to mNTray mNCellsPerTray * loadParams(runnumber) * Gene suggested use Previous year Oct 1st to be safe to grep the correct table * modify double timeres_tof() itray-1, imodule-1, icell-1 * add call writePPPAHistogram from Bassam * remove those tem debug lines Co-authored-by: Zaochen Ye <zye20@rcas6015.rcf.bnl.gov>

* change void loadParams(const int date = 20160913, const int time = 175725, const char* Default_time ) to loadParm(const int runnumber) * change 120, 192 to mNTray mNCellsPerTray * loadParams(runnumber) * Gene suggested use Previous year Oct 1st to be safe to grep the correct table * modify double timeres_tof() itray-1, imodule-1, icell-1 * add call writePPPAHistogram from Bassam * remove those tem debug lines Co-authored-by: Zaochen Ye <zye20@rcas6015.rcf.bnl.gov>

…-bnl#65) * change void loadParams(const int date = 20160913, const int time = 175725, const char* Default_time ) to loadParm(const int runnumber) * change 120, 192 to mNTray mNCellsPerTray * loadParams(runnumber) * Gene suggested use Previous year Oct 1st to be safe to grep the correct table * modify double timeres_tof() itray-1, imodule-1, icell-1 * add call writePPPAHistogram from Bassam * remove those tem debug lines Co-authored-by: Zaochen Ye <zye20@rcas6015.rcf.bnl.gov>

Zaochen Ye added 2 commits July 21, 2021 12:26

change void loadParams(const int date = 20160913, const int time = 17…

b5fa133

…5725, const char* Default_time ) to loadParm(const int runnumber)

change 120, 192 to mNTray mNCellsPerTray

11d7ab4

Morning-Ye requested review from fgeurts, jdbrice and starsdong as code owners July 21, 2021 17:27

loadParams(runnumber)

29a6d34

Gene suggested use Previous year Oct 1st to be safe to grep the corre…

6a2950a

…ct table

Zaochen Ye added 2 commits July 28, 2021 13:01

modify double timeres_tof() itray-1, imodule-1, icell-1

4fbed00

add call writePPPAHistogram from Bassam

b524448

starsdong approved these changes Jul 28, 2021

View reviewed changes

plexoos requested changes Jul 28, 2021

View reviewed changes

StRoot/StBTofUtil/StBTofSimResParams.h Outdated Show resolved Hide resolved

StRoot/StBTofUtil/StBTofSimResParams.h Outdated Show resolved Hide resolved

StRoot/StBTofUtil/StBTofSimResParams.h Outdated Show resolved Hide resolved

remove those tem debug lines

d28a30d

plexoos reviewed Jul 28, 2021

View reviewed changes

Morning-Ye changed the title ~~loadParams(const int runNumber) in StRoot/StBTofUtil/StBTofSimResParams.h~~ update loadParams(const int runNumber) and fix the index issue in StRoot/StBTofUtil/StBTofSimResParams.h Jul 28, 2021

plexoos merged commit d131162 into star-bnl:main Jul 29, 2021

plexoos mentioned this pull request Jul 29, 2021

EbyET0 in simulation #76

Merged

BassamAboona mentioned this pull request Jul 30, 2021

Correcting indexing bug for BTOF resolutions and adding back the call… #54

Closed

genevb mentioned this pull request Sep 9, 2021

let StBTofSimResParams.h to use existed timestamp in bfc chain, instead of re-define a new timestamp #145

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update loadParams(const int runNumber) and fix the index issue in StRoot/StBTofUtil/StBTofSimResParams.h #65

update loadParams(const int runNumber) and fix the index issue in StRoot/StBTofUtil/StBTofSimResParams.h #65

Morning-Ye commented Jul 21, 2021

Morning-Ye commented Jul 26, 2021

genevb commented Jul 26, 2021

Morning-Ye commented Jul 26, 2021 via email

genevb commented Jul 26, 2021 via email

plexoos commented Jul 26, 2021

veprbl commented Jul 27, 2021

Morning-Ye commented Jul 27, 2021

genevb commented Jul 27, 2021 •

edited

starsdong commented Jul 27, 2021

Morning-Ye commented Jul 27, 2021 via email

starsdong commented Jul 27, 2021

Morning-Ye commented Jul 27, 2021 via email

genevb commented Jul 28, 2021 via email

Morning-Ye commented Jul 28, 2021 via email

starsdong commented Jul 28, 2021

starsdong left a comment

Morning-Ye commented Jul 28, 2021 via email

starsdong commented Jul 28, 2021

plexoos commented Jul 28, 2021

Morning-Ye commented Jul 28, 2021 via email

plexoos Jul 28, 2021

plexoos Jul 28, 2021

Morning-Ye Jul 28, 2021

update loadParams(const int runNumber) and fix the index issue in StRoot/StBTofUtil/StBTofSimResParams.h #65

update loadParams(const int runNumber) and fix the index issue in StRoot/StBTofUtil/StBTofSimResParams.h #65

Conversation

Morning-Ye commented Jul 21, 2021

Morning-Ye commented Jul 26, 2021

genevb commented Jul 26, 2021

Morning-Ye commented Jul 26, 2021 via email

genevb commented Jul 26, 2021 via email

plexoos commented Jul 26, 2021

veprbl commented Jul 27, 2021

Morning-Ye commented Jul 27, 2021

genevb commented Jul 27, 2021 • edited

starsdong commented Jul 27, 2021

Morning-Ye commented Jul 27, 2021 via email

starsdong commented Jul 27, 2021

Morning-Ye commented Jul 27, 2021 via email

genevb commented Jul 28, 2021 via email

Morning-Ye commented Jul 28, 2021 via email

starsdong commented Jul 28, 2021

starsdong left a comment

Choose a reason for hiding this comment

Morning-Ye commented Jul 28, 2021 via email

starsdong commented Jul 28, 2021

plexoos commented Jul 28, 2021

Morning-Ye commented Jul 28, 2021 via email

plexoos Jul 28, 2021

Choose a reason for hiding this comment

plexoos Jul 28, 2021

Choose a reason for hiding this comment

Morning-Ye Jul 28, 2021

Choose a reason for hiding this comment

genevb commented Jul 27, 2021 •

edited