Run3-gex74X Try to resolve the overlap in PPS #33760

bsunanda · 2021-05-17T22:48:05Z

PR description:

Try to resolve the overlap in PPS. It changes positions of some of the volumes.

PR validation:

Tested with overlap check tools

if this PR is a backport please specify the original PR and why you need to backport that PR:

Nothing special

cmsbuild · 2021-05-17T22:55:13Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33760/22710

This PR adds an extra 40KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File Configuration/Geometry/python/dict2021Geometry.py modified in PR(s): [DD4hep] start on geometry XML payload producer #33548
- File Geometry/CMSCommonData/data/dd4hep/cmsExtendedGeometry2021.xml modified in PR(s): [DD4hep] start on geometry XML payload producer #33548
- File Geometry/CMSCommonData/data/dd4hep/cmsExtendedGeometry2021FlatMinus05Percent.xml modified in PR(s): [DD4hep] start on geometry XML payload producer #33548
- File Geometry/CMSCommonData/data/dd4hep/cmsExtendedGeometry2021FlatMinus10Percent.xml modified in PR(s): [DD4hep] start on geometry XML payload producer #33548
- File Geometry/CMSCommonData/data/dd4hep/cmsExtendedGeometry2021FlatPlus05Percent.xml modified in PR(s): [DD4hep] start on geometry XML payload producer #33548
- File Geometry/CMSCommonData/data/dd4hep/cmsExtendedGeometry2021FlatPlus10Percent.xml modified in PR(s): [DD4hep] start on geometry XML payload producer #33548
- File Geometry/CMSCommonData/data/dd4hep/cmsExtendedGeometry2021ZeroMaterial.xml modified in PR(s): [DD4hep] start on geometry XML payload producer #33548
- File Geometry/CMSCommonData/python/cmsExtendedGeometry2021FlatMinus05PercentXML_cfi.py modified in PR(s): [DD4hep] start on geometry XML payload producer #33548
- File Geometry/CMSCommonData/python/cmsExtendedGeometry2021FlatMinus10PercentXML_cfi.py modified in PR(s): [DD4hep] start on geometry XML payload producer #33548
- File Geometry/CMSCommonData/python/cmsExtendedGeometry2021FlatPlus05PercentXML_cfi.py modified in PR(s): [DD4hep] start on geometry XML payload producer #33548
- File Geometry/CMSCommonData/python/cmsExtendedGeometry2021FlatPlus10PercentXML_cfi.py modified in PR(s): [DD4hep] start on geometry XML payload producer #33548
- File Geometry/CMSCommonData/python/cmsExtendedGeometry2021XML_cfi.py modified in PR(s): [DD4hep] start on geometry XML payload producer #33548
- File Geometry/CMSCommonData/python/cmsExtendedGeometry2021ZeroMaterialXML_cfi.py modified in PR(s): [DD4hep] start on geometry XML payload producer #33548

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-33760/22710/code-format.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33760/22710/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

bsunanda · 2021-05-17T23:04:54Z

@cmsbuild Please test

cmsbuild · 2021-05-17T23:06:33Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33760/22711

This PR adds an extra 40KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File Configuration/Geometry/python/dict2021Geometry.py modified in PR(s): [DD4hep] start on geometry XML payload producer #33548
- File Geometry/CMSCommonData/data/dd4hep/cmsExtendedGeometry2021.xml modified in PR(s): [DD4hep] start on geometry XML payload producer #33548
- File Geometry/CMSCommonData/data/dd4hep/cmsExtendedGeometry2021FlatMinus05Percent.xml modified in PR(s): [DD4hep] start on geometry XML payload producer #33548
- File Geometry/CMSCommonData/data/dd4hep/cmsExtendedGeometry2021FlatMinus10Percent.xml modified in PR(s): [DD4hep] start on geometry XML payload producer #33548
- File Geometry/CMSCommonData/data/dd4hep/cmsExtendedGeometry2021FlatPlus05Percent.xml modified in PR(s): [DD4hep] start on geometry XML payload producer #33548
- File Geometry/CMSCommonData/data/dd4hep/cmsExtendedGeometry2021FlatPlus10Percent.xml modified in PR(s): [DD4hep] start on geometry XML payload producer #33548
- File Geometry/CMSCommonData/data/dd4hep/cmsExtendedGeometry2021ZeroMaterial.xml modified in PR(s): [DD4hep] start on geometry XML payload producer #33548
- File Geometry/CMSCommonData/python/cmsExtendedGeometry2021FlatMinus05PercentXML_cfi.py modified in PR(s): [DD4hep] start on geometry XML payload producer #33548
- File Geometry/CMSCommonData/python/cmsExtendedGeometry2021FlatMinus10PercentXML_cfi.py modified in PR(s): [DD4hep] start on geometry XML payload producer #33548
- File Geometry/CMSCommonData/python/cmsExtendedGeometry2021FlatPlus05PercentXML_cfi.py modified in PR(s): [DD4hep] start on geometry XML payload producer #33548
- File Geometry/CMSCommonData/python/cmsExtendedGeometry2021FlatPlus10PercentXML_cfi.py modified in PR(s): [DD4hep] start on geometry XML payload producer #33548
- File Geometry/CMSCommonData/python/cmsExtendedGeometry2021XML_cfi.py modified in PR(s): [DD4hep] start on geometry XML payload producer #33548
- File Geometry/CMSCommonData/python/cmsExtendedGeometry2021ZeroMaterialXML_cfi.py modified in PR(s): [DD4hep] start on geometry XML payload producer #33548

cmsbuild · 2021-05-17T23:06:54Z

A new Pull Request was created by @bsunanda (Sunanda Banerjee) for master.

It involves the following packages:

Configuration/Geometry
Geometry/CMSCommonData
Geometry/VeryForwardData
SimG4Core/Geometry

@civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth, @srimanob, @kpedro88 can you please review it and eventually sign? Thanks.
@vargasa, @makortel, @cvuosalo, @forthommel, @jan-kaspar, @rovere, @Martin-Grunewald, @fabiocos, @slomeo this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

cmsbuild · 2021-05-18T01:40:16Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ae2b76/15146/summary.html
COMMIT: 0351120
CMSSW: CMSSW_12_0_X_2021-05-17-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/33760/15146/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: 6 differences found in the comparisons
DQMHistoTests: Total files compared: 37
DQMHistoTests: Total histograms compared: 2648242
DQMHistoTests: Total failures: 7
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2648213
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 36 files compared)
Checked 155 log files, 37 edm output root files, 37 DQM output files
TriggerResults: no differences found

cvuosalo · 2021-05-18T16:51:28Z

...try/VeryForwardData/data/CTPPS_Timing_Horizontal_Pot/2021/v1/CTPPS_Timing_Horizontal_Pot.xml

+      <rSolid name="Bottom_Wall_nocut"/>
+      <rSolid name="Window_Cut"/>
+      <Translation x="0*mm" y="0*mm" z="[bottom_wall_thickness]/2-[cut_depth]/2"/>
+  </SubtractionSolid>


Indentation is wrong here.

cvuosalo · 2021-05-18T16:52:16Z

...try/VeryForwardData/data/CTPPS_Timing_Horizontal_Pot/2021/v1/CTPPS_Timing_Horizontal_Pot.xml

@@ -0,0 +1,60 @@
+<?xml version="1.0" encoding="UTF-8"?>


Did you want to change this file? I did a diff and found no significant changes.

I was working with 2 xml files and changed them several times. At the end I was happy by changing only 1 file. Let me delete the unchanged file and redo the cfi's

cmsbuild · 2021-05-18T22:09:28Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ae2b76/15178/summary.html
COMMIT: dedc2ba
CMSSW: CMSSW_12_0_X_2021-05-18-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/33760/15178/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: 8 differences found in the comparisons
DQMHistoTests: Total files compared: 37
DQMHistoTests: Total histograms compared: 2648242
DQMHistoTests: Total failures: 12
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 2648207
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.004 KiB( 36 files compared)
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
Checked 155 log files, 37 edm output root files, 37 DQM output files
TriggerResults: no differences found

civanch · 2021-05-19T08:45:40Z

+1

srimanob · 2021-05-19T08:53:08Z

Could you please clarify the naming convention you are using, i.e 2018 in "CTPPS_Diamond_2018" with "2021". I understand that 2021 is used to represent Run-3. Why don't we start to make CTPPS_Diamond_2021?

'Geometry/VeryForwardData/data/CTPPS_Diamond_2018/CTPPS_Diamond_Detector_Assembly/2021/v1/CTPPS_Diamond_Detector_Assembly.xml'

civanch · 2021-05-19T13:42:07Z

Likely Phat is right in general but situation is even more complicate. If one looks into https://cmssdt.cern.ch/lxr/source/Geometry/VeryForwardData/data/ there are many directories and single files with some chaos in names. One concern: CTPPS is an obsolete historical name, which do not corresponds to real detector. Another concern: should we use sub-directories or take plane files?

There was an agreement to reduce number "CTPPS" inside file names in new files and read it out where possible. Of course we cannot change "CTPPS_Diamond_2018" or "CTPPS_Diamond_2017", because it is not easy to find out all places where these files are used.

Now question: what files from this complex structure are used in Extended2021 geometry for SIM and for RECO? What is an optimal solution in a long time? The new file may be moved inside 2021 directory, would it be the most optimal solution for today?

forthommel · 2021-05-19T14:01:46Z

This is probably where the PPS RECO people can be ping'ed!
@fabferro @jan-kaspar

fabferro · 2021-05-19T14:21:46Z

@civanch we are considering a reordering in this directory while implementing the real new geometry for run3 detectors.
This should happen soon, probably not in a single step.
Unfortunately there are several files to be considered and the way they have been added have not followed good criteria.

civanch · 2021-05-19T14:53:09Z

@fabferro , we need to make a choice: we agree with the fix of @bsunanda , or we ask to move new files to another directory. This needs to be decided just today. Can you, please, comment?

cvuosalo · 2021-05-19T14:57:02Z

I think this PR should be merged as is. The re-organization of file names is a separate issue. It's important to get the overlaps fixed now.

fabferro · 2021-05-19T15:10:25Z

I think this PR should be merged as is. The re-organization of file names is a separate issue. It's important to get the overlaps fixed now.

Yes. I agree. Please go on with this fix.

civanch · 2021-05-19T15:14:51Z

@srimanob , would you agree that directory structure will be sort out in the following PRs?

srimanob · 2021-05-19T15:15:37Z

+Upgrade

This PR is to fix overlap in PPS. As agreed in the PR discussion, this PR fix is merged first. Then we expect the re-organization of filenames, directories in the new Run-3 geometry.

cmsbuild · 2021-05-19T15:15:40Z

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

qliphy · 2021-05-19T15:43:28Z

+1

Run3-gex74Y Backport #33760 to remove overlap in PPS

Try to resolve the overlap in PPS

aa91b4f

cmsbuild added this to the CMSSW_12_0_X milestone May 17, 2021

cmsbuild added code-checks-pending geometry-pending orp-pending pending-signatures simulation-pending tests-pending upgrade-pending labels May 17, 2021

cmsbuild added code-checks-rejected and removed code-checks-pending labels May 17, 2021

Code check

0351120

cmsbuild added code-checks-pending and removed code-checks-rejected labels May 17, 2021

cmsbuild added tests-started and removed tests-pending labels May 17, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels May 17, 2021

cmsbuild added tests-approved and removed tests-started labels May 18, 2021

cvuosalo reviewed May 18, 2021

View reviewed changes

Remove the unmodified file

dedc2ba

cmsbuild removed code-checks-approved tests-approved labels May 18, 2021

cmsbuild added geometry-approved and removed geometry-pending labels May 18, 2021

cmsbuild added tests-approved and removed tests-started labels May 18, 2021

cmsbuild added simulation-approved and removed simulation-pending labels May 19, 2021

cmsbuild added fully-signed upgrade-approved and removed pending-signatures upgrade-pending labels May 19, 2021

cmsbuild added orp-approved and removed orp-pending labels May 19, 2021

cmsbuild merged commit 3f20827 into cms-sw:master May 19, 2021

bsunanda pushed a commit to bsunanda/cmssw that referenced this pull request May 19, 2021

Backport cms-sw#33760 to remove overlap in PPS

530030b

bsunanda mentioned this pull request May 19, 2021

Run3-gex74Y Backport #33760 to remove overlap in PPS #33789

Merged

cmsbuild mentioned this pull request May 20, 2021

setCovarianceVersion for all lost tracks #33790

Merged

cmsbuild added a commit that referenced this pull request May 23, 2021

Merge pull request #33789 from bsunanda/Run3-gex74Y

b0e3637

Run3-gex74Y Backport #33760 to remove overlap in PPS

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run3-gex74X Try to resolve the overlap in PPS #33760

Run3-gex74X Try to resolve the overlap in PPS #33760

bsunanda commented May 17, 2021

cmsbuild commented May 17, 2021

bsunanda commented May 17, 2021

cmsbuild commented May 17, 2021

cmsbuild commented May 17, 2021

cmsbuild commented May 18, 2021

cvuosalo May 18, 2021

cvuosalo May 18, 2021

bsunanda May 18, 2021

cmsbuild commented May 18, 2021

civanch commented May 19, 2021

srimanob commented May 19, 2021

civanch commented May 19, 2021

forthommel commented May 19, 2021

fabferro commented May 19, 2021

civanch commented May 19, 2021

cvuosalo commented May 19, 2021

fabferro commented May 19, 2021

civanch commented May 19, 2021

srimanob commented May 19, 2021

cmsbuild commented May 19, 2021

qliphy commented May 19, 2021

Run3-gex74X Try to resolve the overlap in PPS #33760

Run3-gex74X Try to resolve the overlap in PPS #33760

Conversation

bsunanda commented May 17, 2021

PR description:

PR validation:

if this PR is a backport please specify the original PR and why you need to backport that PR:

cmsbuild commented May 17, 2021

bsunanda commented May 17, 2021

cmsbuild commented May 17, 2021

cmsbuild commented May 17, 2021

cmsbuild commented May 18, 2021

Comparison Summary

cvuosalo May 18, 2021

Choose a reason for hiding this comment

cvuosalo May 18, 2021

Choose a reason for hiding this comment

bsunanda May 18, 2021

Choose a reason for hiding this comment

cmsbuild commented May 18, 2021

Comparison Summary

civanch commented May 19, 2021

srimanob commented May 19, 2021

civanch commented May 19, 2021

forthommel commented May 19, 2021

fabferro commented May 19, 2021

civanch commented May 19, 2021

cvuosalo commented May 19, 2021

fabferro commented May 19, 2021

civanch commented May 19, 2021

srimanob commented May 19, 2021

cmsbuild commented May 19, 2021

qliphy commented May 19, 2021