Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backport patches for embedding (SL20c) #498

Merged
merged 13 commits into from
May 10, 2023
Merged

Backport patches for embedding (SL20c) #498

merged 13 commits into from
May 10, 2023

Conversation

zhux97
Copy link
Contributor

@zhux97 zhux97 commented Feb 26, 2023

Patches needed for Run18 Isobar (st_physics) embedding are assembled. The patched codes are extracted from PR #466, PR #481.

@zhux97
Copy link
Contributor Author

zhux97 commented Feb 26, 2023

Please find some detail about this PR in the following slides
https://drupal.star.bnl.gov/STAR/system/files/embedding_status_20230226_PR498_0.pdf

@plexoos plexoos added backport Label for backport changes applied to SLXXy branches SL20c labels Feb 26, 2023
Copy link
Contributor

@genevb genevb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been asked by @starsdong to review this, but I've avoided commenting on other such PRs for reasons that are not best discussed here.

The changes to StTpcRTSHitMaker codes in this commit do two things of note to me:

  1. The modifications made in February 2020 to address Coverity and compiler issues have been removed as if they were to be ignored.
  2. The entire maxHitsPerSector and maxBinZeroHits portions of code introduced long ago have been removed, which can potentially lead to different physics results as these were designed to exclude problematic events from reconstruction. This is essentially a modification of the reconstruction code from the version used in official productions.

@zhux97
Copy link
Contributor Author

zhux97 commented Mar 18, 2023 via email

@genevb
Copy link
Contributor

genevb commented Mar 20, 2023

Hi Gene, thanks for bringing up these issues. For my educations, are these issues affecting the official real data productions? or only embedding productions?

As a generalization, the maxHitsPerSector and maxBinZeroHits cuts are in use in official productions. Specific to SL20c, which was used for official BFC productions of datasets only in Runs 17 and 18, the default cut values are in place and no study was performed to see if there were problematic events that the cuts helped us handle. That does not imply that there were no such problematic events, as the cuts would have done their job without much fanfare (only a minor notice in the production log files), and the potential is there for jobs to get hung on tracking for those events. There is also the possibility that not a single event was affected by the cuts for Runs 17 and 18 as the problematic events were more prominent in low energy collisions.

I don't understand why we would make an effort to specifically modify the code to remove these cuts.

-Gene

@zhux97
Copy link
Contributor Author

zhux97 commented Mar 21, 2023 via email

@genevb
Copy link
Contributor

genevb commented Mar 21, 2023 via email

@klendathu2k
Copy link
Contributor

klendathu2k commented Mar 21, 2023 via email

@zhux97
Copy link
Contributor Author

zhux97 commented Apr 19, 2023

Hi Gene, Jason, Dmitri, the StTpcRTSHitMaker.cxx has been updated with Xin's PR #510. Now the high occupancy protection code is restored.

@starsdong
Copy link
Member

Dmitri, could you please take a look at the CI build? It seems to be related to the environment setup? Thanks

@plexoos
Copy link
Member

plexoos commented Apr 19, 2023

Yes, will do

@starsdong
Copy link
Member

Hi Dmitri, I wonder whether you have any findings from the CI build why it failed. Would be great this can be solved asap and we can proceed with the embedding production. Thanks

@starsdong
Copy link
Member

Hi Dmitri, I am curious whether you have any findings on the failure of the CI build in this PR. The embedding team is eagerly waiting for this to be merged. Thanks

plexoos and others added 10 commits May 8, 2023 23:08
```
git diff SL20c_2..SL23a_0 -- docker/ .github/workflows/ Dockerfile tests/ asps/rexe/Cons* asps/rexe/*LinkDef*  mgr/Cons* mgr/config/ StRoot/macros/rootlogo*.C StRoot/macros/.rootrc > patch.diff
git am < patch.diff
```
Since this is done manually for the the generated classes (`StarGeometry` and
`Geometry`), we need to turn off the automatic dictionary generation in
`StarVMC/Geometry` triggered otherwise by the `StarGeoLinkDef.h` file.

cherry-pick 8378339
@plexoos
Copy link
Member

plexoos commented May 10, 2023

The changes in this PR submitted by Xianglei pass the tests (as expected). It is somewhat unexpected and worth noting here that the SL20c branch also had to be patched to include the PicoVtxFXT option introduced after SL20c in order for the tests to work on Run 20 data samples.

@plexoos plexoos merged commit 3f9857f into star-bnl:SL20c May 10, 2023
60 checks passed
@zhux97
Copy link
Contributor Author

zhux97 commented May 11, 2023 via email

@genevb
Copy link
Contributor

genevb commented May 11, 2023 via email

@plexoos
Copy link
Member

plexoos commented May 11, 2023

Hi Gene, actually I created the SL20c_3 tag yesterday, just forgot to announce.

@genevb
Copy link
Contributor

genevb commented May 11, 2023 via email

@plexoos
Copy link
Member

plexoos commented May 11, 2023

Right. I've tagged the latest commit on main with SL20c_3 now. I hope this is what we want.

@genevb
Copy link
Contributor

genevb commented May 11, 2023

Right. I've tagged the latest commit on main with SL20c_3 now. I hope this is what we want.

@klendathu2k , @zhux97 : is the latest version of star-mcgen what we want? Or just continue with what it was before now (tagged as SL20c_2)?

@zhux97
Copy link
Contributor Author

zhux97 commented May 12, 2023 via email

@genevb
Copy link
Contributor

genevb commented May 12, 2023 via email

@zhux97
Copy link
Contributor Author

zhux97 commented May 12, 2023 via email

@klendathu2k
Copy link
Contributor

klendathu2k commented May 12, 2023 via email

@genevb
Copy link
Contributor

genevb commented May 17, 2023 via email

@plexoos
Copy link
Member

plexoos commented May 17, 2023

SL20c_3 now points to the same commit as SL20c_2

@genevb
Copy link
Contributor

genevb commented May 18, 2023 via email

@zhux97
Copy link
Contributor Author

zhux97 commented May 18, 2023 via email

@zhux97
Copy link
Contributor Author

zhux97 commented May 18, 2023 via email

@genevb
Copy link
Contributor

genevb commented May 18, 2023 via email

@genevb
Copy link
Contributor

genevb commented May 18, 2023 via email

@zhux97
Copy link
Contributor Author

zhux97 commented May 18, 2023 via email

@genevb
Copy link
Contributor

genevb commented May 19, 2023 via email

@zhux97
Copy link
Contributor Author

zhux97 commented May 19, 2023 via email

@genevb
Copy link
Contributor

genevb commented May 19, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport Label for backport changes applied to SLXXy branches SL20c
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants