Try to fix GeometryProducer and G4e examples #38864

VinInn · 2022-07-26T11:30:15Z

status
works for run1 geometry (at least does not crash)
does not work for run2 (at least 2022)

the test does not find products (getByLabel broken?)

updated status:
should work for all Eras and geometry models.
the propagator "old" test needs a "refresh": it was based on the early use of Geane for Muon reconstruction that has been obsoleted years ago. It cannot even work at the moment as current propagator is limited to 2m

VinInn · 2022-07-26T11:31:49Z

TrackPropagation/Geant4e/test/Geant4ePropagatorAnalyzer.cc

  }
  LogDebug("Geant4e") << "G4e -- Got simTracks of size " << simTracks->size();
+  std::cout << "Geant4e " << "G4e -- Got simTracks of size " << simTracks->size()  << std::endl;


this one produces:

%MSG-w Geant4e: Geant4ePropagatorAnalyzer:propAna 26-Jul-2022 13:23:00 CEST Run: 1 Event: 1 No tracks found %MSG Geant4e G4e -- Got simTracks of size ----- Begin Fatal Exception 26-Jul-2022 13:23:00 CEST----------------------- An exception of category 'ProductNotFound' occurred while [0] Processing Event run: 1 lumi: 1 event: 1 stream: 0 [1] Running path 'g4AnalPath' [2] Calling method for module Geant4ePropagatorAnalyzer/'propAna' Exception Message: Principal::getByLabel: Found zero products matching all criteria Looking for type: std::vector<SimTrack> Looking for module label: g4SimHits Looking for productInstanceName:

cmsbuild · 2022-07-26T11:38:07Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38864/31276

This PR adds an extra 24KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File TrackPropagation/Geant4e/test/Geant4e_example_cfg.py modified in PR(s): update unit tests for G4e refitter package #38738

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38864/31276/code-format.patch
e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38864/31276/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2022-07-26T11:46:45Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38864/31277

This PR adds an extra 24KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File TrackPropagation/Geant4e/test/Geant4e_example_cfg.py modified in PR(s): update unit tests for G4e refitter package #38738

cmsbuild · 2022-07-26T11:46:59Z

A new Pull Request was created by @VinInn (Vincenzo Innocente) for master.

It involves the following packages:

SimG4Core/GeometryProducer (simulation)
TrackPropagation/Geant4e (simulation)

@cmsbuild, @civanch, @mdhildreth can you please review it and eventually sign? Thanks.
@fabiocos, @VourMa, @makortel, @felicepantaleo, @JanFSchulte, @rovere, @VinInn, @bsunanda, @lecriste, @ebrondol, @mtosi, @mmusich, @slomeo this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy, @rappoccio you are the release manager for this.

cms-bot commands are listed here

VinInn · 2022-07-26T11:48:00Z

@cmsbuild, please test

VinInn · 2022-07-26T11:49:36Z

let's the test fail

mmusich · 2022-07-26T11:47:00Z

TrackPropagation/Geant4e/test/Geant4ePropagatorAnalyzer.cc

  if (!simVertices.isValid()) {
-    LogWarning("Geant4e") << "No tracks found" << std::endl;
-    return;
+    LogWarning("Geant4e") << "No verticess found" << std::endl;


thanks for spotting.
Once we find how to get the products will fix this as well

Probably unrelated, but about

the test does not find products (getByLabel broken?)

Shouldn't be tokens used instead? https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideEDMGetDataFromEvent#get_ByToken

AFAIK getByLabel is deprecated, but I would have expected a different error message.

AFAIK getByLabel is deprecated, but I would have expected a different error message.

Correct, and I would have expected too to see a different exception (complaining about missing consumes() calls).

deprecated should not mean not working... (or message should indeed say something different)

Does one really has to consume each single product used?
and create tokens etc just for a single very stupid analyzer?
One cannot just say I consume everything in the input file?
I understand products to be produced, but for those in the input file is a bit abusive to ask all those boilerplate....

is there a script that generate the boilerplate?
I will not write it by hand!

Does one really has to consume each single product used?

Yes.

and create tokens etc just for a single very stupid analyzer?

Yes.

One cannot just say I consume everything in the input file?

No.

I understand products to be produced, but for those in the input file is a bit abusive to ask all those boilerplate....

The consumes information is used also for input file to read in only those branches that are actually needed by the module.

is there a script that generate the boilerplate?

I'm not aware of any.

I've added

@@ -147,6 +147,9 @@ Geant4ePropagatorAnalyzer::Geant4ePropagatorAnalyzer(const edm::ParameterSet &iC thePropagator(nullptr), G4VtxSrc_(iConfig.getParameter<edm::InputTag>("G4VtxSrc")), G4TrkSrc_(iConfig.getParameter<edm::InputTag>("G4TrkSrc")) { + + consumes(G4TrkSrc_); +

in the constructor.

getByLabel still produces the very same error as above

The point of the migration is to use getByToken instead of getByLabel

Just to note that the exception message has been improved for this particular corner case in #38875.

About

@@ -147,6 +147,9 @@ Geant4ePropagatorAnalyzer::Geant4ePropagatorAnalyzer(const edm::ParameterSet &iC thePropagator(nullptr), G4VtxSrc_(iConfig.getParameter<edm::InputTag>("G4VtxSrc")), G4TrkSrc_(iConfig.getParameter<edm::InputTag>("G4TrkSrc")) { + + consumes(G4TrkSrc_); +

in this case the consumes() call would need an explicit type (because it can't infer it from anywhere) along

@@ -147,6 +147,9 @@ Geant4ePropagatorAnalyzer::Geant4ePropagatorAnalyzer(const edm::ParameterSet &iC thePropagator(nullptr), G4VtxSrc_(iConfig.getParameter<edm::InputTag>("G4VtxSrc")), G4TrkSrc_(iConfig.getParameter<edm::InputTag>("G4TrkSrc")) { + + consumes<std::vector<SimTrack>>(G4TrkSrc_); +

In that case the getByLabel() should still work (even if getByToken() is strongly favored). The consumes() call itself has been required since 2016.

VinInn · 2022-07-26T15:39:53Z

On 26 Jul, 2022, at 4:34 PM, Matti Kortelainen ***@***.***> wrote: The consumes information is used also for input file to read in only those branches that are actually needed by the module.

would not be enough something in the PoolSource such ```keep *_g4SimHits_*_SIM``` ? Anyhow that's a disgression. Apparenlty no one else is botherer by having more boilerplate than actual code in their Analyzer (or nobody is actually doing analysis in cmssw anyhow) v.

cmsbuild · 2022-07-26T16:03:59Z

-1

Failed Tests: UnitTests
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-1c3342/26464/summary.html
COMMIT: 2816210
CMSSW: CMSSW_12_5_X_2022-07-26-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/38864/26464/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found errors in the following unit tests:

---> test testG4Refitter had ERRORS

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 51
DQMHistoTests: Total histograms compared: 3667670
DQMHistoTests: Total failures: 14
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3667634
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 50 files compared)
Checked 210 log files, 47 edm output root files, 51 DQM output files
TriggerResults: no differences found

VinInn · 2022-07-27T15:54:24Z

what about this (including the modifier)?

VinInn · 2022-07-27T15:54:36Z

@cmsbuild, please test

makortel · 2022-07-27T15:59:10Z

what about this (including the modifier)?

Looks ok to me (including the modifier).

cmsbuild · 2022-07-27T16:06:23Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38864/31296

This PR adds an extra 28KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File TrackPropagation/Geant4e/test/BuildFile.xml modified in PR(s): update unit tests for G4e refitter package #38738
- File TrackPropagation/Geant4e/test/Geant4e_example_cfg.py modified in PR(s): update unit tests for G4e refitter package #38738

cmsbuild · 2022-07-27T16:06:47Z

Pull request #38864 was updated. @civanch, @mdhildreth can you please check and sign again.

cmsbuild · 2022-07-27T19:46:53Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-1c3342/26489/summary.html
COMMIT: c8c63c9
CMSSW: CMSSW_12_5_X_2022-07-27-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/38864/26489/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 10 differences found in the comparisons
DQMHistoTests: Total files compared: 51
DQMHistoTests: Total histograms compared: 3667670
DQMHistoTests: Total failures: 19
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 3667628
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.004 KiB( 50 files compared)
DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
Checked 210 log files, 47 edm output root files, 51 DQM output files
TriggerResults: no differences found

civanch · 2022-07-27T20:40:23Z

+1

configuration of SIM magnetic field is more complicate in g4SImHits than is needed for propagation but the use of clone in any case is more correct than it was before.

cmsbuild · 2022-07-27T20:40:43Z

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. @perrotta, @dpiparo, @qliphy, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

perrotta · 2022-07-28T05:47:04Z

@VinInn the PR is still [RFC], and even if fully signed it will be considered for being merged after than that sign get removed from the PR title.

By the way the PR description says

updated status:
should work for all Eras and geometry models.
the "old" propagator test needs a "refresh"

which kind of confirms that there is still some work in progress for the "old" propagator

VinInn · 2022-07-29T12:11:02Z

old refers to test not propagator.
IMHO this PR can be merged if Trk-Pog coordinators agree.

btw the reason the simple test does not propagate beyond 2m is trivial
https://cmssdt.cern.ch/dxr/CMSSW/source/TrackPropagation/Geant4e/src/Geant4ePropagator.cc#334

mmusich · 2022-07-29T13:08:27Z

IMHO this PR can be merged if Trk-Pog coordinators agree.

FWIW, +1

qliphy · 2022-07-30T01:17:46Z

+1

goemtry ok (for run1). not able to get event content

37ea99a

cmsbuild added this to the CMSSW_12_5_X milestone Jul 26, 2022

cmsbuild added code-checks-pending orp-pending pending-signatures simulation-pending tests-pending labels Jul 26, 2022

VinInn commented Jul 26, 2022

View reviewed changes

VinInn mentioned this pull request Jul 26, 2022

update unit tests for G4e refitter package #38738

Merged

cmsbuild added code-checks-rejected and removed code-checks-pending labels Jul 26, 2022

code format

84ea5ec

cmsbuild added code-checks-pending and removed code-checks-rejected labels Jul 26, 2022

add unit test

2816210

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 26, 2022

cmsbuild added tests-started and removed tests-pending labels Jul 26, 2022

mmusich reviewed Jul 26, 2022

View reviewed changes

cmsbuild added tests-rejected and removed tests-started labels Jul 26, 2022

fix config for Run3

386cf0d

cmsbuild added tests-started and removed tests-pending labels Jul 27, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 27, 2022

makortel mentioned this pull request Jul 27, 2022

Throw GetByLabelWithoutRegistration exception also when no module in a job registered consumption of a given branch #38875

Merged

cmsbuild added tests-approved and removed tests-started labels Jul 27, 2022

cmsbuild added fully-signed simulation-approved and removed simulation-pending pending-signatures labels Jul 27, 2022

VinInn changed the title ~~[RFC] try to fix GeometryProducer and G4e examples~~ Try to fix GeometryProducer and G4e examples Jul 29, 2022

cmsbuild added orp-approved and removed orp-pending labels Jul 30, 2022

cmsbuild merged commit af61d7b into cms-sw:master Jul 30, 2022

This was referenced Jul 30, 2022

[12.4.X] fix GeometryProducer and G4e examples #38906

Merged

G4e refitter configuration broken by dd4hep #38739

Closed

mmusich mentioned this pull request Aug 15, 2022

introduce G4e refitter in the tracker alignment machinery #39070

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try to fix GeometryProducer and G4e examples #38864

Try to fix GeometryProducer and G4e examples #38864

VinInn commented Jul 26, 2022 •

edited

Loading

VinInn Jul 26, 2022

cmsbuild commented Jul 26, 2022

cmsbuild commented Jul 26, 2022

cmsbuild commented Jul 26, 2022

VinInn commented Jul 26, 2022

VinInn commented Jul 26, 2022

mmusich Jul 26, 2022

VinInn Jul 26, 2022

mmusich Jul 26, 2022

makortel Jul 26, 2022

VinInn Jul 26, 2022

VinInn Jul 26, 2022

makortel Jul 26, 2022

VinInn Jul 27, 2022

mmusich Jul 27, 2022

makortel Aug 1, 2022

VinInn commented Jul 26, 2022 via email

cmsbuild commented Jul 26, 2022

VinInn commented Jul 27, 2022

VinInn commented Jul 27, 2022

makortel commented Jul 27, 2022

cmsbuild commented Jul 27, 2022

cmsbuild commented Jul 27, 2022

cmsbuild commented Jul 27, 2022

civanch commented Jul 27, 2022

cmsbuild commented Jul 27, 2022

perrotta commented Jul 28, 2022

VinInn commented Jul 29, 2022 •

edited

Loading

mmusich commented Jul 29, 2022

qliphy commented Jul 30, 2022

Try to fix GeometryProducer and G4e examples #38864

Try to fix GeometryProducer and G4e examples #38864

Conversation

VinInn commented Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

cmsbuild commented Jul 26, 2022

cmsbuild commented Jul 26, 2022

cmsbuild commented Jul 26, 2022

VinInn commented Jul 26, 2022

VinInn commented Jul 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VinInn commented Jul 26, 2022 via email

cmsbuild commented Jul 26, 2022

Unit Tests

Comparison Summary

VinInn commented Jul 27, 2022

VinInn commented Jul 27, 2022

makortel commented Jul 27, 2022

cmsbuild commented Jul 27, 2022

cmsbuild commented Jul 27, 2022

cmsbuild commented Jul 27, 2022

Comparison Summary

civanch commented Jul 27, 2022

cmsbuild commented Jul 27, 2022

perrotta commented Jul 28, 2022

VinInn commented Jul 29, 2022 • edited Loading

mmusich commented Jul 29, 2022

qliphy commented Jul 30, 2022

VinInn commented Jul 26, 2022 •

edited

Loading

VinInn commented Jul 29, 2022 •

edited

Loading