search

star-bnl / star-sw

Core software for STAR experiment
26 stars 63 forks source link

dEdx Run XXI calibrations (1st pass) #353

Closed fisyak closed 1 year ago

starsdong commented 2 years ago

Hi Yuri,

Thank you for putting this PR. My understanding is that this will also include the dEdx correction for heavier particles that we see in previous iTPC production. Not sure if this is late, do you think it is possible that you can discuss this and your test results at the S&C management meeting tomorrow? Thanks

iraklic commented 2 years ago

Hello Yuri,

Few questions:

  1. There are some empty files like this StarDb/Calibrations/tpc/tpcAltroParams.20180426.121919.C, are these links that show as empty files?
  2. There are some root files, are those for template fits?

Irakli

fisyak commented 2 years ago

Hi Xin, to finish verification I need to have this corrections to be propagated to dev version and run on the calibration sample. After this verification I plan to present the modifications to TPC and S&C groups. This version should work with heavy particles (up to triton). For He3 and He4 I need additional modification on which I am still working. Hi Irakli, root-files were committed by mistake. I have removed them. I need a new table in MySQL and for time being I am using soft links for these tables. .

plexoos commented 1 year ago

Here is how these changes can be tested before they are merged into main.

  1. Go to the latest GitHub actions page for this PR (https://github.com/star-bnl/star-sw/actions/runs/2352599614) and download the zip archive with already built libraries using the star-sw-root5-build link under Artifacts
  2. Unpack the downloaded zip archive to get a single tar file
  3. Build a singularity (or docker) image from the tar file
  4. Run any command in the container

On SDCC only singularity is available, so the above would look like this (actual commands):

$ curl -L -u token:<YOUR_GH_TOKEN>  https://api.github.com/repos/star-bnl/star-sw/actions/artifacts/246519433/zip | gunzip -d > star-sw-root5-build.tar
$ singularity build star-sw-root5-build.sif docker-archive://star-sw-root5-build.tar

The above setup should take only a few minutes. Now run your bfc.C chain. I am using one of the nightly tests as an example:

$ singularity run -e -B /gpfs01:/gpfs01 star-sw-root5-build.sif root4star -b -q 'bfc.C(10, \"P2018a,StiCA,btof,mtd,PicoVtxDefault,BEmcChkStat,QAalltrigs,OSpaceZ2,OGridLeak3D,-hitfilt\", \"/gpfs01/star/rcf/test/daq/2018/136/st_physics_19136012_raw_1000016.daq\")'
fisyak commented 1 year ago

Dmitry, the test of compilation and run I do before committing any changes. The test which you have proposed have been done during the standard checks. The system which you have developed is blocking release of dev library and this does block verification of these commits with respect to all calibration samples which I need to do. .

plexoos commented 1 year ago

Xin, whenever you think this is ready to be merged just let the Infrastructure team know and it will be merged.

starsdong commented 1 year ago

Hi Dmitry A. could you please take a look at the proposed changes to the StDetectorDbMaker etc.? If you have no objection, please feel free to approve the request.

Hi Dmitri S., on the suggested changes you mentioned, do the current CI build tests cover it already?

Thanks

plexoos commented 1 year ago

on the suggested changes you mentioned, do the current CI build tests cover it already?

Here is the list of tests covered by the CI https://github.com/star-bnl/star-sw/blob/main/tests/joblist.json

dmarkh commented 1 year ago

Hi Xin,

StDetectorDbMaker is Yuri's code and is used by TPC only. I cannot review a PR that contains many changes to TPC-specific codes as I am not an expert in TPC.

Meanwhile, I am still working on six new tables requested by Yuri. Would have been more efficient if IDL files were coming in a separate PR though.

-Dmitry

On Fri, May 20, 2022 at 12:12 PM Xin Dong @.***> wrote:

Hi Dmitry A. could you please take a look at the proposed changes to the StDetectorDbMaker etc.? If you have no objection, please feel free to approve the request.

Hi Dmitri S., on the suggested changes you mentioned, do the current CI build tests cover it already?

Thanks

— Reply to this email directly, view it on GitHub https://github.com/star-bnl/star-sw/pull/353#issuecomment-1133083189, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHWGXQG7NAMNDWTW3AQ3W5LVK62VZANCNFSM5WGSCEGQ . You are receiving this because your review was requested.Message ID: @.***>

starsdong commented 1 year ago

Ah, Dmitri, my understanding of your suggestion is that Yuri can work on the downstream tests (which depends on these changes) following your suggest while this PR is under review. The test from the proponent is not the requirement (presumably CI builds cover the tests for this PR) for this PR to be approved. Correct?

plexoos commented 1 year ago

Xin, in my comment I was not asking Yuri to run any specific test. Using one of the tests as an example, I just showed how one can run additional root4star jobs (if needed) by using the libraries already built from this PR branch. It was my understanding that Yuri has not finalized this work and there is more to come.

Anyway, if Yuri can run his jobs on the farm only when the libraries are in /afs then the only option for us is to merge this now as is.

dmarkh commented 1 year ago

All eight tables requested by Yuri are now implemented in the database: https://online.star.bnl.gov/dbExplorer/#Calibrations/tpc/

  1. itpcDeadFEE / itpcDeadFEE / 192
  2. TpcAdcCorrection6MDF / MDFCorrection4 / 50
  3. TpcLengthCorrectionMDN / MDFCorrection / 50
  4. TpcnTbk / tpcCorrection / 50
  5. TpcnPad / tpcCorrection / 50
  6. TpcPadPedRMS / tpcCorrection / 50
  7. GatingGrid / tpcCorrection / 50
  8. TpcAdcCorrectionC / tpcCorrection / 50

Thanks, Dmitry

starsdong commented 1 year ago

Hi Dmitry, I think to move the PR forward, it will need your approval to proceed. Would please help review and comment/approve this PR? Thanks

dmarkh commented 1 year ago

Hi Xin,

As I explained earlier, StDetectorDbMaker is not a generic DB API code and I do not maintain it. Yuri maintains StDetectorDbMaker, AFAIK. Generic DB codes I maintain are: StDbLib (core DB API library, not ROOT-dependent), StDbBroker (broker between StDbLib and St_db_Maker) and St_db_Maker (framework API, ROOT-dependent). Also, I manage db-stored data and db-stored descriptors. The only "code" I can review in this request is IDLs describing data structures. I did create appropriate db tables and STAR db API descriptors for those IDLs, but I know nothing about other code changes. Can't be made responsible for something I have no relation to.

Thanks for the understanding, Dmitry

On Thu, May 26, 2022 at 2:23 PM Xin Dong @.***> wrote:

Hi Dmitry, I think to move the PR forward, it will need your approval to proceed. Would please help review and comment/approve this PR? Thanks

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because your review was requested.Message ID: @.***>

plexoos commented 1 year ago

Xin, I've looked through the changes in StDetectorDbMaker. There are mainly new class wrappers around the new DB structs with some methods added... I don't see any significant changes in the logic. I believe the primary concern may be in the new calibration values inserted in the database and in the CINT files in StarDb/ . These may affect reconstructed tracks of course. But unless there is an independent QA check, we can only rely on the nightly tests to make sure there are no unexpected changes. So, I'd say we merge and keep an eye on the metric we have.

genevb commented 1 year ago

we can only rely on the nightly tests to make sure there are no unexpected changes.

Just a follow-up on this note from @plexoos : Yesterday, the nightly tests did identify an issue with running simulations in old libraries caused by the initially proposed and implemented (on Tuesday) database table modifications. @fisyak 's 82b0736f2bfa545dc57b1d131d9aefa4c8ab8574 commit this morning is a response to the observed issue. I expect the issue is resolved by today's commit and database change that @dmarkh helped with, but I haven't seen a confirming test of that yet. The nightly test should confirm tonight if no one tests it before then.

starsdong commented 1 year ago

Thank you all for the comments. Dmitri, I would suggest let us merge this request. We will watch the nightly test and other further tests.