[DD4hep] MagneticField Builder Shapes Use Cleanup - Use dd4hep::Solid Directly #31578

ianna · 2020-09-25T08:43:44Z

PR description:

Addressing #31569

Use DD4hep Shapes and their accessors

This is done in preparation of removing the extra classes that duplicate the DD4hep ones:
https://cmssdt.cern.ch/lxr/source/DetectorDescription/DDCMS/interface/DDShapes.h

@namapane - The functionality is unchanged - essentially, it's a one to one copy from DDCMS. The comparison tests pass.
The PseudoTrap workaround is untouched. Please, check the units conversion and the angle restriction.

If it's ok with you, I'd like to integrate this PR asap. It will allow me to proceed with removal of the DDShapes.h and DDShapes.cc. The MF builder is the last one that using it.

There is an inconsistency in getting a half Z (AIDASoft/DD4hep#714). We may need to revisit this code depending on the issue outcome later.

PR validation:

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

Before submitting your pull requests, make sure you followed this checklist:

verify that the PR is really intended for the chosen branch
verify that changes follow CMS Naming, Coding, And Style Rules
verify that the PR passes the basic test procedure suggested in the CMSSW PR instructions

cmsbuild · 2020-09-25T08:44:07Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-09-25T08:50:56Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31578/18623

This PR adds an extra 16KB to repository

cmsbuild · 2020-09-25T08:51:21Z

A new Pull Request was created by @ianna (Ianna Osborne) for master.

It involves the following packages:

MagneticField/GeomBuilder

@perrotta, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@namapane 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

ianna · 2020-09-25T08:56:44Z

please test

cmsbuild · 2020-09-25T08:57:05Z

The tests are being triggered in jenkins.

CMSSW_11_2_X_2020-09-24-2300/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/9573/console Started: 2020/09/25 10:59

namapane · 2020-09-25T09:10:29Z

Uhm. buildBox.icc etc. are also used in the non-DD4Hep version that is still the default. I am a bit uncomfortable with diverging in the two implementations, unless there is a clear timeline on when we will make the DD4Hep version default and drop the non-DD4Hep one.
Also I would have to test all existing geometries built from both DB and xml to check for possible side effects, can you wait for a few days for these checks before merging ?

namapane · 2020-09-25T09:16:49Z

And actually what is the exact point of moving the content of the .icc files in the cc? From a quick look I did not spot code changes... I would avoid duplicating this code if that is not strictly necessary.

cmsbuild · 2020-09-25T10:12:21Z

+1
Tested at: 21c69bf
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d181c9/9573/summary.html
CMSSW: CMSSW_11_2_X_2020-09-24-2300
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-09-25T10:12:24Z

Comparison job queued.

ianna · 2020-09-25T10:33:26Z

Uhm. buildBox.icc etc. are also used in the non-DD4Hep version that is still the default. I am a bit uncomfortable with diverging in the two implementations, unless there is a clear timeline on when we will make the DD4Hep version default and drop the non-DD4Hep one.
Also I would have to test all existing geometries built from both DB and xml to check for possible side effects, can you wait for a few days for these checks before merging ?

@namapane - Sure, no problem. As far as I understand, the 20th October is a deadline to start full validation. I'd like to restrict the API beforehand.

The differences are the shape APIs. For example, here is a new version:

  DDBox box(solid.solid());
  // Old DD needs mm to cm conversion, but DD4hep needs no conversion.
  // convertUnits should be defined appropriately.
  double halfX = convertUnits(box.x());
  double halfY = convertUnits(box.y());
  double halfZ = convertUnits(box.z());

vs an old version:

  DDBox box(solid);
  // Old DD needs mm to cm conversion, but DD4hep needs no conversion.
  // convertUnits should be defined appropriately.
  double halfX = convertUnits(box.halfX());
  double halfY = convertUnits(box.halfY());
  double halfZ = convertUnits(box.halfZ());

After #31577 and #31571 are merged MF builder remains the only package that is still using the DDShapes wrapper classes.

If you prefer, we can move it under your control :-) https://cmssdt.cern.ch/lxr/source/DetectorDescription/DDCMS/interface/DDShapes.h
https://cmssdt.cern.ch/lxr/source/DetectorDescription/DDCMS/src/DDShapes.cc

In any case, you are welcome to take over this PR.

namapane · 2020-09-25T11:17:58Z

@ianna I see; I would actually prefer to modify the existing functions to pass the list of parameters for each case (the solution that was eventually adopted for buildPseudoTrap) so that we don't need to duplicate code. It should be rather straightforward but I need to carve out a couple of days for that... What is your target date for restricting the API?

What is your deadline to restrict the API?

ianna · 2020-09-25T11:34:19Z

@ianna I see; I would actually prefer to modify the existing functions to pass the list of parameters for each case (the solution that was eventually adopted for buildPseudoTrap) so that we don't need to duplicate code. It should be rather straightforward but I need to carve out a couple of days for that... What is your target date for restricting the API?

What is your deadline to restrict the API?

@namapane - I was hoping to finish it this week since I need to concentrate on another unrelated project, but it can wait a couple of days, for sure.

cmsbuild · 2020-09-25T11:45:58Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d181c9/9573/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: 2539438
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2539415
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
Checked 149 log files, 22 edm output root files, 35 DQM output files

cvuosalo · 2020-09-25T16:15:17Z

MagneticField/GeomBuilder/src/DD4hep_volumeHandle.cc

+using DDTrap = dd4hep::Trap;
+using DDTubs = dd4hep::Tube;
+using DDCons = dd4hep::ConeSegment;
+using DDTruncTubs = dd4hep::TruncatedTube;


If the above five lines were put into DetectorDescription/DDCMS/interface/DDShapes.h, then this PR wouldn't be needed. Or would there be some problem?

cvuosalo · 2020-09-25T16:17:27Z

MagneticField/GeomBuilder/src/DD4hep_volumeHandle.cc

-#include "buildTrap.icc"
-#include "buildTubs.icc"
-#include "buildCons.icc"
+/*


I think the code duplication below should be avoided. I don't understand why the icc files can't be used as they are.

That was my question (see above) but Ianna pointed out to slight changes in interface, eg x() becoming halfX()
My suggestion is to extract the relevant parameters for each shape in *volumeHandle.cc and pass them to the functions in the .icc files, as done already in buildPseudoTrap.icc. This way the actual implementation remains where it is and we just need to adjust the callers.

slava77 · 2020-09-26T00:51:01Z

assign geometry

since quite a bit of the discussion and possible updates are related to geometry changes, I'd rather have a signature provided explicitly. The reco signature may follow after, assuming @namapane is happy with the changes.

cmsbuild · 2020-09-26T00:51:27Z

New categories assigned: geometry

@Dr15Jones,@cvuosalo,@mdhildreth,@makortel,@ianna,@civanch you have been requested to review this Pull request/Issue and eventually sign? Thanks

ianna · 2020-10-01T16:03:34Z

@namapane - I was wondering what to do with this PR. I can just move the DDShapes.h/.cc under MagneticField/GeomBuilder/src/ to decouple the other PRs.

namapane · 2020-10-01T20:03:43Z

@ianna - for what I'm concerned this PR should be recoded (ie closed and replaced by another one) refactoring the code that is being duplicated in this PR so that it does not need to be duplicated. It is a really easy task (just pass float parametes instead of shapes to the functions that are being replicated). As promised I plan to work on that.
I do not see much point in releasing this PR as is by also moving here classes whose dependency was, in any case, introduced because of the migration to DD4Hep itself (and not by me). That it will make subsequent cleanup unnecessarily more complicated.
That is just my opinion and you can of course go on if you prefer, Although I am sure that, on the path of release, others will object to the duplication of code snippets...

ianna · 2020-10-02T07:18:50Z

@ianna - for what I'm concerned this PR should be recoded (ie closed and replaced by another one) refactoring the code that is being duplicated in this PR so that it does not need to be duplicated. It is a really easy task (just pass float parametes instead of shapes to the functions that are being replicated). As promised I plan to work on that.
I do not see much point in releasing this PR as is by also moving here classes whose dependency was, in any case, introduced because of the migration to DD4Hep itself (and not by me). That it will make subsequent cleanup unnecessarily more complicated.
That is just my opinion and you can of course go on if you prefer, Although I am sure that, on the path of release, others will object to the duplication of code snippets...

@namapane - thanks! OK, I'm closing this PR. Would you object moving the DDShapes.h/.cc under MagneticField/GeomBuilder/src/ to decouple the other pending PRs? Thanks.

namapane · 2020-10-02T07:44:27Z

I'm working on this right now

namapane

Done, #31649.

namapane · 2020-10-02T10:55:17Z

MagneticField/GeomBuilder/src/DD4hep_volumeHandle.cc

+  double rIn = convertUnits(tubs.rMin());
+  double rOut = convertUnits(tubs.rMax());
+  double startPhi = convertDegToRad(tubs.startPhi());
+  double deltaPhi = convertDegToRad(tubs.endPhi()) - startPhi;


I am looking into this in the context of #31578 and I wonder why you converted to rad here? To me looks like these are in rad already.
[edit: they are in rad indeed. I removed the conversion from the re-implementation in #31649

use dd4hep shape accessors

21c69bf

cmsbuild added this to the CMSSW_11_2_X milestone Sep 25, 2020

cmsbuild added code-checks-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Sep 25, 2020

ianna mentioned this pull request Sep 25, 2020

[DD4hep] MagnetidField Geometry Builder to Use dd4hep::Solid #31569

Closed

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 25, 2020

cmsbuild added tests-started and removed tests-pending labels Sep 25, 2020

cmsbuild added tests-approved and removed tests-started labels Sep 25, 2020

cmsbuild added comparison-available and removed comparison-pending labels Sep 25, 2020

ianna mentioned this pull request Sep 25, 2020

[DD4hep] Remove Shape Wrapper Classes #31581

Merged

cvuosalo reviewed Sep 25, 2020

View reviewed changes

cmsbuild added the geometry-pending label Sep 26, 2020

ianna mentioned this pull request Sep 28, 2020

[DD4hep] Filtered View is not a Shape #31603

Closed

ianna closed this Oct 2, 2020

namapane mentioned this pull request Oct 2, 2020

Remove dependency from DDCMS/interface/DDShapes.h #31649

Merged

namapane reviewed Oct 2, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DD4hep] MagneticField Builder Shapes Use Cleanup - Use dd4hep::Solid Directly #31578

[DD4hep] MagneticField Builder Shapes Use Cleanup - Use dd4hep::Solid Directly #31578

ianna commented Sep 25, 2020

cmsbuild commented Sep 25, 2020

cmsbuild commented Sep 25, 2020

cmsbuild commented Sep 25, 2020

ianna commented Sep 25, 2020

cmsbuild commented Sep 25, 2020 •

edited

Loading

namapane commented Sep 25, 2020

namapane commented Sep 25, 2020

cmsbuild commented Sep 25, 2020

cmsbuild commented Sep 25, 2020

ianna commented Sep 25, 2020

namapane commented Sep 25, 2020

ianna commented Sep 25, 2020

cmsbuild commented Sep 25, 2020

cvuosalo Sep 25, 2020

cvuosalo Sep 25, 2020

namapane Sep 25, 2020

slava77 commented Sep 26, 2020

cmsbuild commented Sep 26, 2020

ianna commented Oct 1, 2020

namapane commented Oct 1, 2020

ianna commented Oct 2, 2020

namapane commented Oct 2, 2020

namapane left a comment

namapane Oct 2, 2020 •

edited

Loading

[DD4hep] MagneticField Builder Shapes Use Cleanup - Use dd4hep::Solid Directly #31578

[DD4hep] MagneticField Builder Shapes Use Cleanup - Use dd4hep::Solid Directly #31578

Conversation

ianna commented Sep 25, 2020

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 Sep 25, 2020

cmsbuild commented Sep 25, 2020

cmsbuild commented Sep 25, 2020

ianna commented Sep 25, 2020

cmsbuild commented Sep 25, 2020 • edited Loading

namapane commented Sep 25, 2020

namapane commented Sep 25, 2020

cmsbuild commented Sep 25, 2020

cmsbuild commented Sep 25, 2020

ianna commented Sep 25, 2020

namapane commented Sep 25, 2020

ianna commented Sep 25, 2020

cmsbuild commented Sep 25, 2020

cvuosalo Sep 25, 2020

Choose a reason for hiding this comment

cvuosalo Sep 25, 2020

Choose a reason for hiding this comment

namapane Sep 25, 2020

Choose a reason for hiding this comment

slava77 commented Sep 26, 2020

cmsbuild commented Sep 26, 2020

ianna commented Oct 1, 2020

namapane commented Oct 1, 2020

ianna commented Oct 2, 2020

namapane commented Oct 2, 2020

namapane left a comment

Choose a reason for hiding this comment

namapane Oct 2, 2020 • edited Loading

Choose a reason for hiding this comment

cmsbuild commented Sep 25, 2020 •

edited

Loading

namapane Oct 2, 2020 •

edited

Loading