Skip to content

Fix missing CaloParticles from pileup #47682

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

Merged
merged 4 commits into from
Apr 1, 2025
Merged

Conversation

AuroraPerego
Copy link
Contributor

PR description:

Since CMSSW_15_0_0_pre2, there are no CaloParticles from pileup due to a wrong ioread rule that led to misreading the pileup files.
This PR fixes the ioread rule, which required temporarily adding back a data member to the simTracks collection (I haven't found a better solution).

In addition, a processModifier to consider also CaloParticles from pileup in the associators (leading to the creation of tracksters from pileup) has been added.

PR validation:

Tested on 29888.208 and printing the number of caloparticles and simclusters.

@waredjeb @felicepantaleo @rovere

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 25, 2025

cms-bot internal usage

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47682/44227

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @AuroraPerego for master.

It involves the following packages:

  • Configuration/ProcessModifiers (operations)
  • Configuration/PyReleaseValidation (pdmv, upgrade)
  • SimCalorimetry/HGCalSimProducers (simulation, upgrade)
  • SimDataFormats/Track (simulation)

@AdrianoDee, @Moanwar, @antoniovilela, @civanch, @cmsbuild, @davidlange6, @DickyChant, @fabiocos, @kpedro88, @mandrenguyen, @mdhildreth, @miquork, @rappoccio, @srimanob, @subirsarkar can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @VinInn, @VourMa, @apsallid, @bsunanda, @cseez, @denizsun, @fabiocos, @hatakeyamak, @lgray, @makortel, @martinamalberti, @missirol, @mmusich, @mtosi, @pfs, @rovere, @salimcerci, @sameasy, @sethzenz, @slomeo, @vandreev11, @youyingli this is something you requested to watch as well.
@antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

@Moanwar
Copy link
Contributor

Moanwar commented Mar 25, 2025

test parameters:
workflow_opts = -w upgrade
workflows = 29888.208

@Moanwar
Copy link
Contributor

Moanwar commented Mar 25, 2025

type bug-fix

@Moanwar
Copy link
Contributor

Moanwar commented Mar 25, 2025

@cmsbuild, please test

Comment on lines 18 to 23
if (newObj->crossedBoundaryOld())
tmp.setCrossedBoundaryVars(
true, newObj->getIDAtBoundary(), newObj->getPositionAtBoundary(), newObj->getMomentumAtBoundary());
else
tmp.setCrossedBoundaryVars(
false, newObj->getIDAtBoundary(), newObj->getPositionAtBoundary(), newObj->getMomentumAtBoundary());
Copy link
Contributor

Choose a reason for hiding this comment

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

it is possible to access the member variables from the old version of the object. in this case, you should be able to do this:

tmp.setCrossedBoundaryVars(
            onfile.crossedBoundary_, newObj->getIDAtBoundary(), newObj->getPositionAtBoundary(), newObj->getMomentumAtBoundary());

then you can avoid restoring the old member variable in the current object definition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks a lot! I'll implement this

@@ -4,22 +4,24 @@
<class name="CoreSimTrack" ClassVersion="10">
<version ClassVersion="10" checksum="3936841839"/>
</class>
<class name="SimTrack" ClassVersion="13">
<class name="SimTrack" ClassVersion="14">
<version ClassVersion="14" checksum="2517401874"/>
<version ClassVersion="13" checksum="1912247222"/>
<version ClassVersion="12" checksum="3470347245"/>
<version ClassVersion="11" checksum="1785575744"/>
<version ClassVersion="10" checksum="1430205451"/>
<ioread sourceClass = "SimTrack" version="[-12]" targetClass="SimTrack" source="" target="">
Copy link
Contributor

Choose a reason for hiding this comment

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

In addition to what Kevin wrote, the source and target attributes should list all the SimTrack member variables that are read from the up-to-12 objects, and written to for 13-onwards objects, respectively. (listing in source may be required to access onfile.crossedBoundary_)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed, I needed also a source="bool crossedBoundary_", thanks!

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47682/44234

@civanch
Copy link
Contributor

civanch commented Mar 30, 2025

@AuroraPerego , @kpedro88 , let us rediscuss how this extra flag is used and how used other parameters. To me it is look like a temporary solution, if we have troubles with one flag now, then very soon we will need other modifications and new problems.

We may merge this PR first, make an issue in github, and continue discussion.

@kpedro88
Copy link
Contributor

@civanch I'm not sure what you mean. This PR just fixes a bug from a previous PR and makes it easier to run test workflows in the future.

@civanch
Copy link
Contributor

civanch commented Mar 31, 2025

+1

@AdrianoDee
Copy link
Contributor

+pdmv

@Moanwar
Copy link
Contributor

Moanwar commented Mar 31, 2025

+Upgrade

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @sextonkennedy, @antoniovilela, @rappoccio, @mandrenguyen (and backports should be raised in the release meeting by the corresponding L2)

@mandrenguyen
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 8c8fe33 into cms-sw:master Apr 1, 2025
12 checks passed
@dan131riley
Copy link

There appears to be some problem with this in the ROOT6 builds--the IBs tracking the ROOT6 master have many failures:

----- Begin Fatal Exception 02-Apr-2025 15:30:52 CEST-----------------------
An exception of category 'FileReadError' occurred while
   [0] Processing  Event run: 1 lumi: 1 event: 4 stream: 1
   [1] Running path 'HLTAnalyzerEndpath'
   [2] Prefetching for module L1TRawToDigi/'hltGtStage2Digis'
   [3] Prefetching for module RawDataCollectorByLabel/'rawDataCollector'
   [4] Prefetching for module SiStripDigiToRawModule/'SiStripDigiToRaw'
   [5] Calling method for module MixingModule/'mix'
   [6] Processing  Event run: 1 lumi: 1 event: 4 stream: 1
   [7] Running path 'HLTAnalyzerEndpath'
   [8] Prefetching for module L1TRawToDigi/'hltGtStage2Digis'
   [9] Prefetching for module RawDataCollectorByLabel/'rawDataCollector'
   [10] Prefetching for module SiStripDigiToRawModule/'SiStripDigiToRaw'
   [11] Calling method for module MixingModule/'mix'
   [12] Calling method for module MixingModule/'mix' (probably inside some kind of mixing module)
   [13] While reading from source std::vector<SimTrack> g4SimHits '' SIM
   [14] Reading branch SimTracks_g4SimHits__SIM.
   Additional Info:
      [a] Fatal Root Error: @SUB=TStreamerInfo::InsertArtificialElements
For class SimTrack in StreamerInfo 11 is missing the source data member `crossedBoundary_` when trying to apply the rule:
   type=read sourceClass="SimTrack" targetClass="SimTrack" version="[-12]" source="bool crossedBoundary_; int idAtBoundary_; ROOT::Math::LorentzVector<ROOT::Math::PxPyPzE4D<float> > positionAtBoundary_; ROOT::Math::LorentzVector<ROOT::Math::PxPyPzE4D<float> > momentumAtBoundary_; int igenpart; " target="trackInfo_, idAtBoundary_, positionAtBoundary_, momentumAtBoundary_" code="{
        // set crossedBoundary infos
        newObj->setCrossedBoundaryVars(onfile.crossedBoundary_, onfile.idAtBoundary_, onfile.positionAtBoundary_, onfile.momentumAtBoundary_);
        // set isPrimary info of trackInfo_
        if (onfile.igenpart != -1)
          newObj->setIsPrimary();
        // it's not possible to set the isFromBackScattering info for old simTracks
    }" 

----- End Fatal Exception -------------------------------------------------

@AuroraPerego
Copy link
Contributor Author

There appears to be some problem with this in the ROOT6 builds--the IBs tracking the ROOT6 master have many failures:

Apparently the crossedBoundary_ variable has been added with the ClassVersion 12, so setCrossedBoundaryVars cannot be called on files with ClassVersion 11.

@kpedro88
Copy link
Contributor

kpedro88 commented Apr 2, 2025

@dan131riley thanks for reporting this. It looks like that workflow is opening a very old file:

Successfully opened file root://eoscms.cern.ch//eos/cms/store/user/cmsbuild/store/relval/CMSSW_10_6_0/RelValHydjetQ_B12_5020GeV_2018/GEN-SIM/106X_upgrade2018_realistic_v4-v1/10000/B280A6F0-DCAB-EF4D-A859-7A3FD8B1F584.root

The field crossedBoundary_ was introduced in #32673 (11_3_X) and removed in #46979 (15_0_X). The introduction of this field created class version 12, i.e. the field did not exist in class version 11. Therefore, the ioread rule should not apply to version 11, i.e. version="[-12]" in this PR needs to be changed to version="[12]". (I am not sure if a different rule is needed for earlier versions...)

@AuroraPerego can you prepare a bug fix PR and test this old workflow explicitly?

@AuroraPerego
Copy link
Contributor Author

sure

@makortel
Copy link
Contributor

makortel commented Apr 2, 2025

(I am not sure if a different rule is needed for earlier versions...)

If the default construction of the SimTrack for the trackInfo_ is ok for the -11 versions, I'd expect ROOT's automatic schema evolution to be sufficient.

@AuroraPerego
Copy link
Contributor Author

If the default construction of the SimTrack for the trackInfo_ is ok for the -11 versions, I'd expect ROOT's automatic schema evolution to be sufficient.

The only information that can be set is the primary / not primary bit, since the crossed boundary and back scattering information were not there at the time. I think it should be set because it's used in other places in the SimTrack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants