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

Add time to HGCAL CaloParticles #46678

Merged
merged 3 commits into from
Dec 2, 2024
Merged

Conversation

AuroraPerego
Copy link
Contributor

@AuroraPerego AuroraPerego commented Nov 12, 2024

PR description:

Following the MtdTruthAccumulator.cc logic, the SimVertex time has also been added to the HGCAL CaloParticles.
This is needed because the information coming from the SimVertexCollection of the CaloParticles from pileup is lost if not saved in the CaloTruthAccumulator.cc.
The name of the method used to assign the time has been changed to the more appropriate setSimTime.
The correction of the time for oot pileup has been removed since the SimVertex time takes that already into account.

PR validation:

Tested on wfs 29888.203 (classical mixing) and 29888.99 (premix) enabling the CaloParticles from pileup (they are disabled by default).
Example to test it:

test parameters:
    - workflow_opts= -w upgrade
    - workflow = 29888.99,29888.203

FYI @waredjeb @felicepantaleo @rovere

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 12, 2024

cms-bot internal usage

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • RecoHGCal/TICL (reconstruction, upgrade)
  • SimDataFormats/CaloAnalysis (simulation)
  • SimGeneral/CaloAnalysis (simulation)
  • SimGeneral/MixingModule (simulation)

@Moanwar, @civanch, @cmsbuild, @jfernan2, @kpedro88, @mandrenguyen, @mdhildreth, @srimanob, @subirsarkar can you please review it and eventually sign? Thanks.
@ReyerBand, @apsallid, @bsunanda, @fabiocos, @felicepantaleo, @forthommel, @hatakeyamak, @lecriste, @makortel, @martinamalberti, @missirol, @mmusich, @rovere, @sameasy, @slomeo, @sobhatta, @thomreis, @wang0jin, @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

@fwyzard
Copy link
Contributor

fwyzard commented Nov 12, 2024

allow @AuroraPerego test rights

@@ -223,6 +229,7 @@ CaloTruthAccumulator::CaloTruthAccumulator(const edm::ParameterSet &config,
: messageCategory_("CaloTruthAccumulator"),
maximumPreviousBunchCrossing_(config.getParameter<unsigned int>("maximumPreviousBunchCrossing")),
maximumSubsequentBunchCrossing_(config.getParameter<unsigned int>("maximumSubsequentBunchCrossing")),
bunchSpacing_(config.getParameter<unsigned int>("bunchspace")),
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, just a small comment—I don't see the usage of bunchSpacing. Maybe I'm missing something? Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes sorry, it was a leftover from previous changes. I've removed it.

@@ -293,7 +287,7 @@ void SimTrackstersProducer::produce(edm::Event& evt, const edm::EventSetup& es)
// Create a Trackster from the object entering HGCal
if (cp.g4Tracks()[0].crossedBoundary()) {
regr_energy = cp.g4Tracks()[0].getMomentumAtBoundary().energy();
float time = cp.g4Tracks()[0].getPositionAtBoundary().t();
float time = cp.g4Tracks()[0].getPositionAtBoundary().t() * 1e9;
Copy link
Contributor

@jfernan2 jfernan2 Nov 12, 2024

Choose a reason for hiding this comment

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

Is this 1e9 a conversion to ns? If so, perhaps set it as a constant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's from seconds to ns, I've changed it to use the constant s from CLHEP.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Pull request #46678 was updated. @Moanwar, @civanch, @cmsbuild, @jfernan2, @kpedro88, @mandrenguyen, @mdhildreth, @srimanob, @subirsarkar can you please check and sign again.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Milestone for this pull request has been moved to CMSSW_15_0_X. Please open a backport if it should also go in to CMSSW_14_2_X.

@cmsbuild cmsbuild modified the milestones: CMSSW_14_2_X, CMSSW_15_0_X Nov 22, 2024
@Moanwar
Copy link
Contributor

Moanwar commented Dec 2, 2024

+Upgrade

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 2, 2024

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

@mandrenguyen
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 3be60c7 into cms-sw:master Dec 2, 2024
11 of 12 checks passed
@mmusich
Copy link
Contributor

mmusich commented Dec 3, 2024

just for the record, it looks like this PR makes the phase2 HLT timing test crash e.g. log:

----- Begin Fatal Exception 03-Dec-2024 09:28:34 CET-----------------------
An exception of category 'FileOpenError' occurred while
   [0] Calling InputSource::readFile_
   [1] Calling RootInputFileSequence::initTheFile()
   Additional Info:
      [a] Input file file:/data/user/cmsbuild//store/relval/CMSSW_14_1_0_pre6/RelValTTbar_14TeV/GEN-SIM-DIGI-RAW/PU_141X_mcRun4_realistic_v1_STD_2026D110_PU-v3/2810000/07aa8d40-9725-47e8-b476-2dd518958f17.root could not be opened.
      [b] Fatal Root Error: @SUB=TStreamerInfo::BuildCheck

   The StreamerInfo for version 4 of class CaloParticle read from the file ///data/user/cmsbuild//store/relval/CMSSW_14_1_0_pre6/RelValTTbar_14TeV/GEN-SIM-DIGI-RAW/PU_141X_mcRun4_realistic_v1_STD_2026D110_PU-v3/2810000/07aa8d40-9725-47e8-b476-2dd518958f17.root
   has a different checksum than the previously loaded StreamerInfo.
   Reading objects of type CaloParticle from the file ///data/user/cmsbuild//store/relval/CMSSW_14_1_0_pre6/RelValTTbar_14TeV/GEN-SIM-DIGI-RAW/PU_141X_mcRun4_realistic_v1_STD_2026D110_PU-v3/2810000/07aa8d40-9725-47e8-b476-2dd518958f17.root 
   (and potentially other files) might not work correctly.
   Most likely the version number of the class was not properly
   updated [See ClassDef(CaloParticle,4)].

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

@AuroraPerego @felicepantaleo

@rovere
Copy link
Contributor

rovere commented Dec 3, 2024

This PR changes the class layout of both CaloParticle and MTDCaloParticle w/o increasing the class version in the proper xml file.
Aren't we catching this kind of compilation warnings?

@AuroraPerego
Copy link
Contributor Author

Aren't we catching this kind of compilation warnings?

Apparently, we are not now (scram b gives no warnings/errors). I ran scram b updateclassversion and it did not fail, while edmCheckClassVersion generated the new checksum correctly.
I'll test the phase2 HLT timing test with the new checksum, but there is probably an issue with scram not catching it (?).
Maybe @smuzaffar has more insights into this.

mmusich added a commit to mmusich/cmssw that referenced this pull request Dec 3, 2024
@mmusich
Copy link
Contributor

mmusich commented Dec 3, 2024

I'll test the phase2 HLT timing test with the new checksum, but there is probably an issue with scram not catching it (?).

I tested it fixes the phase2 HLT timing and opened #46851

@smuzaffar
Copy link
Contributor

@AuroraPerego thanks for pinging me. buildrules are searching for FWCore/Utilities/scripts/edmCheckClassVersion and if not found then it just skip the class version checks. As #45423 moved this file under FWCore/Reflection/scripts/edmCheckClassVersion that is why edm class checks were not run. I will update the build rules to

@smuzaffar
Copy link
Contributor

cms-sw/cmsdist#9543 should properly catch mismatching class versions

cmsbuild added a commit that referenced this pull request Dec 3, 2024
fix `CaloParticle` class version after #46678
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.