search

star-bnl / star-sw

Core software for STAR experiment
26 stars 63 forks source link

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

Closed Morning-Ye closed 2 years ago

Morning-Ye commented 2 years ago

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

Morning-Ye commented 2 years ago

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

genevb commented 2 years ago

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 commented 2 years ago

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 https://github.com/star-bnl/star-sw/pull/65#issuecomment-886978926, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADSL4INSMMF5CZF6QQVFGGLTZW3YFANCNFSM5AYMYTKQ .

genevb commented 2 years ago

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

plexoos commented 2 years ago

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 commented 2 years ago

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

Morning-Ye commented 2 years ago

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 commented 2 years ago

(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 commented 2 years ago

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 commented 2 years ago

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 https://github.com/star-bnl/star-sw/pull/65#issuecomment-887850762, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADSL4IP3DHG3GU3N3YEZ5N3TZ4RD3ANCNFSM5AYMYTKQ .

starsdong commented 2 years ago

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 commented 2 years ago

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 https://github.com/star-bnl/star-sw/pull/65#issuecomment-887860261, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADSL4IKIKM73KD5HYFXAXZ3TZ4TOBANCNFSM5AYMYTKQ .

genevb commented 2 years ago

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 commented 2 years ago

Hi Gene,

Good suggestion, so the easy solution will be change "const int date = yearNumber100010 + 101;" to "const int date = (yearNumber-1)100010 + 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 https://github.com/star-bnl/star-sw/pull/65#issuecomment-887944186, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADSL4IOMMSOHEJEU3DZKXVTTZ5NUFANCNFSM5AYMYTKQ .

starsdong commented 2 years ago

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

Morning-Ye commented 2 years ago

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 https://github.com/star-bnl/star-sw/pull/65#pullrequestreview-717385634, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADSL4IPI5AA3UYZOGNFO6D3T2BKRFANCNFSM5AYMYTKQ .

starsdong commented 2 years ago

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 commented 2 years ago

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

Morning-Ye commented 2 years ago

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 https://github.com/star-bnl/star-sw/pull/65#issuecomment-888631296, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADSL4IKPNYVEEOJGXTY7P3TT2BYO3ANCNFSM5AYMYTKQ .