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 xml file and python customizer for D49 reco tracker material #29910

Merged
merged 3 commits into from May 20, 2020

Conversation

mtosi
Copy link
Contributor

@mtosi mtosi commented May 19, 2020

PR description:

during the studies of the tracking performance in phase2 targeting the 111X
it turned out that the current material budget used in the track reconstruction is sub-optimal,
in particular in describing the IT material

preliminary results have been presented and shown during the TK DPG meeting last week [1]

a new --very preliminary-- version of the reco tracker material budget has been derived
thanks to the Rovere's workflow using NeutrinoGun events for the D49 geometry

this PR provides the corresponding recoTrackerMaterial.xml file,
as well as the customization function for making use of it instead of the default file (thanks @mmusich)

no changes are expected in the standard workflows,
but this customization function is needed in order to make relval samples

[1]
https://indico.cern.ch/event/879450/contributions/3857982/attachments/2036336/3410814/200512_TRKPOGPH2.pdf

@mtosi
Copy link
Contributor Author

mtosi commented May 19, 2020

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29910/15511

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented May 19, 2020

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/6431/console Started: 2020/05/19 18:45

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @mtosi (mia tosi) for master.

It involves the following packages:

Geometry/TrackerRecoData
SLHCUpgradeSimulations/Configuration

@civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @kpedro88 can you please review it and eventually sign? Thanks.
@makortel, @ebrondol, @fabiocos, @sviret, @VinInn this is something you requested to watch as well.
@silviodonato, @dpiparo you are the release manager for this.

cms-bot commands are listed here

@mmusich
Copy link
Contributor

mmusich commented May 19, 2020

@emiglior @skinnari FYI

@cmsbuild
Copy link
Contributor

+1
Tested at: b38549c
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d38a07/6431/summary.html
CMSSW: CMSSW_11_1_X_2020-05-19-1100
SCRAM_ARCH: slc7_amd64_gcc820

@cmsbuild
Copy link
Contributor

Comparison job queued.

@silviodonato
Copy link
Contributor

upgrade: @kpedro88
geometry: @Dr15Jones @civanch @cvuosalo @ianna @makortel @mdhildreth
simulation: @civanch @mdhildreth
This PR is adding two new files. If you have time to have a look, we might include it in pre8.


if hasattr(process,'XMLIdealGeometryESSource') and hasattr(process.XMLIdealGeometryESSource,'geomXMLFiles'):

print("customizing Tracker Reco Material")
Copy link
Contributor

Choose a reason for hiding this comment

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

would prefer not to have this printout

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cleaned

if hasattr(process,'XMLIdealGeometryESSource') and hasattr(process.XMLIdealGeometryESSource,'geomXMLFiles'):

print("customizing Tracker Reco Material")
process.XMLIdealGeometryESSource.geomXMLFiles.remove(
Copy link
Contributor

Choose a reason for hiding this comment

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

you should probably check if this file is present in the list before removing it, otherwise python will raise a ValueError

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, thanks for the comment

Copy link
Contributor

Choose a reason for hiding this comment

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

it's a rather non-pythonic fix though and also the new code allows for silent failing of the application of the customization function if the file is not present.

I had in mind something like:

diff --git a/SLHCUpgradeSimulations/Configuration/python/customise_TkRecoMaterial.py b/SLHCUpgradeSimulations/Configuration/python/customise_TkRecoMaterial.py
index 8d99be95dad..75e64ce891f 100644
--- a/SLHCUpgradeSimulations/Configuration/python/customise_TkRecoMaterial.py
+++ b/SLHCUpgradeSimulations/Configuration/python/customise_TkRecoMaterial.py
@@ -1,20 +1,21 @@
 '''Customization functions for cmsDriver to change the phase-2 tracker reco material for geometry D49'''
 import FWCore.ParameterSet.Config as cms
-def customizeRecoMaterial(process):
-    
+def customizeTkRecoMaterialD49(process):
+
     ''' will replace one tracker reco material file with another one for geometry D49
-    syntax: --customise SLHCUpgradeSimulations/Configuration/customise_TkRecoMaterial.customizeRecoMaterial
+    syntax: --customise SLHCUpgradeSimulations/Configuration/customise_TkRecoMaterial.customizeTkRecoMaterialD49
     '''
-    
+
     if hasattr(process,'XMLIdealGeometryESSource') and hasattr(process.XMLIdealGeometryESSource,'geomXMLFiles'):
-        
-        print("customizing Tracker Reco Material")
-        process.XMLIdealGeometryESSource.geomXMLFiles.remove(
-            'Geometry/TrackerRecoData/data/PhaseII/TiltedTracker613_MB_2019_04/trackerRecoMaterial.xml'
-        )
-        
-        process.XMLIdealGeometryESSource.geomXMLFiles.append(
-            'Geometry/TrackerRecoData/data/PhaseII/TiltedTracker613_MB_2019_04/v2_ITonly/trackerRecoMaterial.xml'
-        )
-        
+
+        try:
+            process.XMLIdealGeometryESSource.geomXMLFiles.remove(
+                'Geometry/TrackerRecoData/data/PhaseII/TiltedTracker613_MB_2019_04/trackerRecoMaterial.xml'
+            )
+            process.XMLIdealGeometryESSource.geomXMLFiles.append(
+                'Geometry/TrackerRecoData/data/PhaseII/TiltedTracker613_MB_2019_04/v2_ITonly/trackerRecoMaterial.xml'
+            )
+        except ValueError:
+            raise SystemExit("\n\n ERROR! Could not replace trackerRecoMaterial.xml file, please check check if D49 geometry is being used \n\n")
+
     return process

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ciao
thanks for the comment, but the raise SystemExit will make the code exiting, is it what we want ?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, as the customization should simply NOT be run if the geometry is not the correct one.

@@ -0,0 +1,20 @@
'''Customization functions for cmsDriver to change the phase-2 tracker reco material for geometry D49'''
import FWCore.ParameterSet.Config as cms
def customizeRecoMaterial(process):
Copy link
Contributor

Choose a reason for hiding this comment

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

will the new file eventually become the default, so this customize function will only be needed temporarily?

Copy link
Contributor

Choose a reason for hiding this comment

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

@kpedro88 yes, eventually, if validation at https://its.cern.ch/jira/projects/PDMVRELVALS/issues/PDMVRELVALS-79 is satisfactory.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d38a07/6431/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 35
  • DQMHistoTests: Total histograms compared: 2694466
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2694415
  • DQMHistoTests: Total skipped: 49
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
  • Checked 150 log files, 16 edm output root files, 35 DQM output files

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@mtosi
Copy link
Contributor Author

mtosi commented May 20, 2020

by using -geometry Extended2026D49 the customization works as expected (make use of the new .xml file) while using -geometry Extended2026D41 the customization raises the SystemExit w/ the message
ERROR! Could not replace trackerRecoMaterial.xml file, please check if D49 geometry is being used

@mtosi
Copy link
Contributor Author

mtosi commented May 20, 2020

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-29910/15523

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented May 20, 2020

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/6453/console Started: 2020/05/20 10:32

@cmsbuild
Copy link
Contributor

Pull request #29910 was updated. @civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @kpedro88 can you please check and sign again.

@silviodonato
Copy link
Contributor

merge
I think all comments have been implemented.
I wanted to include this PR in the morning IB. In case of problems we can revert it during the day.
Please let us know if you have further comments.
upgrade: @kpedro88
geometry: @Dr15Jones @civanch @cvuosalo @ianna @makortel @mdhildreth
simulation: @civanch @mdhildreth
@mmusich

@cmsbuild cmsbuild merged commit 9da67b4 into cms-sw:master May 20, 2020
@mtosi
Copy link
Contributor Author

mtosi commented May 20, 2020

thanks a lot !

@cmsbuild
Copy link
Contributor

+1
Tested at: f026ac0
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d38a07/6453/summary.html
CMSSW: CMSSW_11_1_X_2020-05-19-2300
SCRAM_ARCH: slc7_amd64_gcc820

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d38a07/6453/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 35
  • DQMHistoTests: Total histograms compared: 2694466
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2694416
  • DQMHistoTests: Total skipped: 49
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
  • Checked 150 log files, 16 edm output root files, 35 DQM output files

@kpedro88
Copy link
Contributor

+upgrade

@cvuosalo
Copy link
Contributor

+1

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.

None yet

6 participants