star-bnl / star-sw

Core software for STAR experiment
26 stars 63 forks source link

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

Closed BassamAboona closed 2 years ago

BassamAboona commented 2 years ago

… to writePPPAHistogram()

fgeurts commented 2 years ago
genevb commented 2 years ago

Do we want to get this fix in place when we freeze SL21c? In any older libraries? Or re-stating, what condition or usage would necessitate a library having this fix in place?

Thanks in advance, -Gene

starsdong commented 2 years ago

I agree with Frank's comment. And the index fix needs to be checked with Zaochen on parameter uploading side. Zaochen?

On the variable naming, I would recommend we use trayId, moduleId, cellId to indicate these physical IDs which starting from 1. While i,j, or itray, imodule, icell can be used as C/C++ array indices. We could avoid confusion/mistakes by using a consistent naming.

P.S. It is probably better to have Zaochen (TOF software coordinator) as one of the codeowners. I can take my name off from this part. We will follow up later on the CODEOWNER list.

Morning-Ye commented 2 years ago

Hi All, thanks for setting my as one of the code owner. I did some tests with the current changes, and realized that the loadParams() StRoot/StBTofUtil/StBTofSimResParams.h actually could not pick up the updated resolution table. I will modify it and upload the updated one in another pull request.

Morning-Ye commented 2 years ago

the index issue actually can be fixed, the current issue is that the loadParams() do not load table corresponding to the timestamp or runnumber

plexoos commented 2 years ago

the index issue actually can be fixed

Can this be clearly demonstrated by any chance? Adding steps to reproduce the problem and then showing the result before and after the change may certainly help.

BassamAboona commented 2 years ago

line 78: size_t index = i 120 + j need to be changed to size_t index = i 192 + j;

Hi Zaochen! I addressed this comment.

Morning-Ye commented 2 years ago

Hi Dmitry, for the updated StRoot/StBTofUtil/StBTofSimResParams.h, do you know how to get the latest "tofSimResParams" table, which was uploaded by me with this macro: "/star/u/zye20/allScripts/serviceWork/TOF_VPD_Calibration/calib/btof_ntuple_maker/params/Run19/auau19.6_4prod_20210428/upload2DB/macros/storeBToFSimResParams.C"

I tried to use change the input date and time for loadParams() and "dbMk->SetDateTime(date,time)" to pick up the new resolution table, however it still read the old one.

dmarkh commented 2 years ago

Hi Dmitry, for the updated StRoot/StBTofUtil/StBTofSimResParams.h, do you know how to get the latest "tofSimResParams" table, which was uploaded by me with this macro: "/star/u/zye20/allScripts/serviceWork/TOF_VPD_Calibration/calib/btof_ntuple_maker/params/Run19/auau19.6_4prod_20210428/upload2DB/macros/storeBToFSimResParams.C"

I tried to use change the input date and time for loadParams() and "dbMk->SetDateTime(date,time)" to pick up the new resolution table, however it still read the old one.

Hello Zaochen, I have used the macro from the dbExplorer, set up your date/time (20160913,175725), and received the correct validity range [20151203.0 - 20180115.2] - as reported by DB browser. See: /star/u/dmitry/4Zaochen/read.C I can't validate the contents though, and I suspect that you might have uploaded wrong data? -Dmitry

Morning-Ye commented 2 years ago

/star/u/dmitry/4Zaochen/read.C

Hi Dmitry, Thanks for this example, it could actually help pickup the correct table from DB. I will test it in the maker. Cheers.

Morning-Ye commented 2 years ago

Hi Dmitry, for the updated StRoot/StBTofUtil/StBTofSimResParams.h, do you know how to get the latest "tofSimResParams" table, which was uploaded by me with this macro: "/star/u/zye20/allScripts/serviceWork/TOF_VPD_Calibration/calib/btof_ntuple_maker/params/Run19/auau19.6_4prod_20210428/upload2DB/macros/storeBToFSimResParams.C" I tried to use change the input date and time for loadParams() and "dbMk->SetDateTime(date,time)" to pick up the new resolution table, however it still read the old one.

Hello Zaochen, I have used the macro from the dbExplorer, set up your date/time (20160913,175725), and received the correct validity range [20151203.0 - 20180115.2] - as reported by DB browser. See: /star/u/dmitry/4Zaochen/read.C I can't validate the contents though, and I suspect that you might have uploaded wrong data? -Dmitry

Hi Dmitry,

Your example also works well in the StRoot/StBTofUtil/StBTofSimResParams.h. I will go to further modify the loadParm() function go let it easily pickup btof resolution tables according to the runnumber instead of a constant timestamp. Cheers!

BassamAboona commented 2 years ago

Since Zaochen's pull request #65 has been approved and it already has the edits that this pull request has, then there is no need for this pull request anymore.