Add basic geometrical accessors to shapes. #697

MarkusFrankATcernch · 2020-08-20T13:12:26Z

BEGINRELEASENOTES

Implement data member accessors for construction parameters in Shapes.h. As discussed in the thread Add DDExtrudedPolygon, DDPolycone, and DDPolyhedra shapes for reading shape parameters cms-sw/cmssw#30931 it was felt that read-only access to the basic construction parameters of a shape would be highly useful.

ENDRELEASENOTES

ianna

@MarkusFrankATcernch - thanks for taking care of it! Just a few points:

I think, we should avoid the unit conversions. We ought to leave it to a user IMHO.
I do not care much if the API is close to G4 or ROOT as long as it is well documented.
Also, I cannot tell if CMS IB checks will be happy with using size_t in place of int which is the most commonly used type. Perhaps, you could try to run clang-tidy with a narrowing conversion check?
Thanks!

ianna · 2020-08-20T14:03:56Z

DDCore/include/DD4hep/Shapes.h

@@ -829,6 +945,27 @@ namespace dd4hep {
    Trap& setDimensions(double z, double theta, double phi,
                        double h1, double bl1, double tl1, double alpha1,
                        double h2, double bl2, double tl2, double alpha2);
+
+    /// Accessor: phi value
+    double phi() const                     { return access()->GetPhi()*dd4hep::deg;    }


@MarkusFrankATcernch - IMHO we need to avoid double conversion, e.g. converting everything to radians (e.g. G4 definition) and then back to degrees (e.g. ROOT definition). We will definitely see some precision loss.

ianna · 2020-08-20T14:17:15Z

DDCore/include/DD4hep/Shapes.h

@@ -1271,6 +1471,27 @@ namespace dd4hep {
    PolyhedraRegular& operator=(PolyhedraRegular&& copy) = default;
    /// Copy Assignment operator
    PolyhedraRegular& operator=(const PolyhedraRegular& copy) = default;
+
+    /// Accessor: Number of edges
+    size_t numEdge()  const                { return access()->GetNedges();           }


I think you are converting signed to an unsigned here. I'd add an s:

int numEdges() const

ianna · 2020-08-20T14:19:04Z

DDCore/include/DD4hep/Shapes.h

+    double deltaPhi() const                { return access()->GetDphi()*dd4hep::deg; }
+
+    /// Accessor: r-min value
+    double z(size_t which) const           { return access()->GetZ(which);           }


I think, it should be int

MarkusFrankATcernch · 2020-08-20T15:06:25Z

@ianna

size_t --> int OK taken into account.
degree-> radians. We sort of have to stay consistent. The constructors also take radians. I would prefer to do the conversion.

More importantly:
Should we check if the object is valid ? This probably really costs. Using ptr()-> instead of access()-> is much cheaper, but also causes SEGV if the object is invalid. The optimizer actually directly leaves code as if the raw pointer is used.

ianna · 2020-08-20T15:11:15Z

@ianna

size_t --> int OK taken into account.

degree-> radians. We sort of have to stay consistent. The constructors also take radians. I would prefer to do the conversion.

More importantly:
Should we check if the object is valid ? This probably really costs. Using ptr()-> instead of access()-> is much cheaper, but also causes SEGV if the object is invalid. The optimizer actually directly leaves code as if the raw pointer is used.

I'd leave the optimisation for later. Most of the CMSSW code uses a dimensions vector any way...

cvuosalo · 2020-08-20T15:34:17Z

The standard has been that DD4hep returns angles as radians. Let's keep that convention.

andresailer

Mostly comments for the docstrings

andresailer · 2020-08-20T13:18:02Z

DDCore/include/DD4hep/Shapes.h

+    /// Accessor: delta-phi value
+    double deltaPhi() const                { return access()->GetDphi()*dd4hep::deg; }
+
+    /// Accessor: r-min value


Suggested change

/// Accessor: r-min value

/// Accessor: z value

andresailer · 2020-08-20T15:55:37Z

DDCore/include/DD4hep/Shapes.h

+    template <typename SOLID> std::vector<double> _extract_vector(const SOLID* solid,
+                                                                  double (SOLID::*extract)(Int_t) const,
+                                                                  Int_t (SOLID::*len)() const)   {
+      std::vector<double> result;


reserve the size (count)

andresailer · 2020-08-20T15:57:07Z

DDCore/include/DD4hep/Shapes.h

+
+    /// Accessor: delta-z value
+    double dZ() const                      { return access()->GetDz();               }
+    /// Accessor: r-min value


Suggested change

/// Accessor: r-min value

/// Accessor: a value

andresailer · 2020-08-20T15:57:16Z

DDCore/include/DD4hep/Shapes.h

+    double dZ() const                      { return access()->GetDz();               }
+    /// Accessor: r-min value
+    double a() const                       { return access()->GetA();                }
+    /// Accessor: r-max value  


Suggested change

/// Accessor: r-max value

/// Accessor: b value

andresailer · 2020-08-20T15:58:36Z

DDCore/include/DD4hep/Shapes.h

+    /// Accessor: delta-phi value
+    double deltaPhi() const                { return access()->GetDphi()*dd4hep::deg; }
+
+    /// Accessor: r-min value


Not sure what this is, but different than r-min

andresailer · 2020-08-20T15:59:26Z

DDCore/include/DD4hep/Shapes.h

+    /// Accessor: delta-phi value
+    double deltaPhi() const                { return access()->GetDphi()*dd4hep::deg; }
+
+    /// Accessor: r-min value


Suggested change

/// Accessor: r-min value

/// Accessor: z value

andresailer · 2020-08-20T15:59:44Z

DDCore/include/DD4hep/Shapes.h

+    /// Accessor: delta-phi value
+    double deltaPhi() const                { return access()->GetDphi()*dd4hep::deg; }
+
+    /// Accessor: r-min value


Suggested change

/// Accessor: r-min value

/// Accessor: z value

andresailer · 2020-08-20T16:00:26Z

DDCore/include/DD4hep/Shapes.h

+      const double* values = access()->GetVertices();
+      return detail::_make_vector(values, 8*2);
+    }
+    /// Accessor: singlke vertex


Suggested change

/// Accessor: singlke vertex

/// Accessor: single vertex

cvuosalo · 2020-08-20T16:35:17Z

Concerning TruncatedTube, "The construction parameters of the TruncatedTube are stored as a string in the object's title" Since the parameters are stored, could you please provide named accessor methods that return each of the construction parameters?

cvuosalo · 2020-08-20T16:43:27Z

DDCore/include/DD4hep/Shapes.h

+    double phi() const                     { return access()->GetPhi()*dd4hep::deg;    }
+    /// Accessor: theta value
+    double theta() const                   { return access()->GetTheta()*dd4hep::deg;  }
+    /// Angle between centers of x edges an y axis at low z


Suggested change

/// Angle between centers of x edges an y axis at low z

/// Angle between centers of x edges and y axis at low z

cvuosalo · 2020-08-20T16:45:10Z

DDCore/include/DD4hep/Shapes.h

+    double theta() const                   { return access()->GetTheta()*dd4hep::deg;  }
+    /// Angle between centers of x edges an y axis at low z
+    double alpha1()  const                 { return access()->GetAlpha1()*dd4hep::deg; }
+    /// Angle between centers of x edges an y axis at low z


Suggested change

/// Angle between centers of x edges an y axis at low z

/// Angle between centers of x edges and y axis at high z

ianna · 2020-08-28T12:24:52Z

@MarkusFrankATcernch - I think, you could introduce a nested namespace for units, for example dd4hep::units::m

[119/607] Building CXX object DDCore/CMakeFiles/DDCore.dir/src/Conditions.cpp.o
1232FAILED: DDCore/CMakeFiles/DDCore.dir/src/Conditions.cpp.o 
1233/cvmfs/sft.cern.ch/lcg/releases/clang/8.0.0-ed577/x86_64-centos7/bin/clang++  -DBOOST_SPIRIT_USE_PHOENIX_V3 -DDD4HEP_USE_TINYXML -DDDCore_EXPORTS -I../DDCore/include -I../DDParsers/include -ftls-model=global-dynamic -Wheader-hygiene -Wno-c++1z-extensions -Winconsistent-missing-override -fdiagnostics-color=auto -Wshadow -Wdeprecated -Wno-long-long -Wformat-security -Wshadow -pedantic -Wextra -Wall -fdiagnostics-color=always -Werror              -pthread -O3 -DNDEBUG -fPIC   -std=c++17 -MD -MT DDCore/CMakeFiles/DDCore.dir/src/Conditions.cpp.o -MF DDCore/CMakeFiles/DDCore.dir/src/Conditions.cpp.o.d -o DDCore/CMakeFiles/DDCore.dir/src/Conditions.cpp.o -c ../DDCore/src/Conditions.cpp
1234../DDCore/src/Conditions.cpp:177:23: error: declaration shadows a variable in namespace 'dd4hep' [-Werror,-Wshadow]
1235  const BasicGrammar* g = access()->data.grammar;
1236                      ^
1237../DDParsers/include/Evaluator/DD4hepUnits.h:191:29: note: previous declaration is here
1238    static constexpr double g = gram;
1239                            ^
1240../DDCore/src/Conditions.cpp:193:12: error: declaration shadows a variable in namespace 'dd4hep' [-Werror,-Wshadow]
1241  KeyMaker m(detector.key(), detail::hash32(value));
1242           ^
1243../DDParsers/include/Evaluator/DD4hepUnits.h:98:29: note: previous declaration is here
1244    static constexpr double m = meter;
1245                            ^
1246../DDCore/src/Conditions.cpp:199:12: error: declaration shadows a variable in namespace 'dd4hep' [-Werror,-Wshadow]
1247  KeyMaker m(detector.key(), item_key);
1248           ^
1249../DDParsers/include/Evaluator/DD4hepUnits.h:98:29: note: previous declaration is here
1250    static constexpr double m = meter;
1251                            ^
1252../DDCore/src/Conditions.cpp:205:12: error: declaration shadows a variable in namespace 'dd4hep' [-Werror,-Wshadow]
1253  KeyMaker m(det, detail::hash32(value));
1254           ^
1255../DDParsers/include/Evaluator/DD4hepUnits.h:98:29: note: previous declaration is here
1256    static constexpr double m = meter;
1257                            ^
12584 errors generated.
1259[121/607] Building CXX object DDCore/C...s/DDCore.dir/src/ConditionsDebug.cpp.o
1260ninja: build stopped: subcommand failed.
1261The command "if [[ "$PYTHON_CHECK" == "yes" ]]; then docker exec -ti CI_container /bin/bash -c "./DD4hep/.dd4hep-ci.d/runPythonChecks.sh"; else docker exec -ti CI_container /bin/bash -c "./DD4hep/.dd4hep-ci.d/compile_and_test.sh"; fi" exited with 1.
1262
1263
1264Done. Your build exited with 1.

cvuosalo · 2020-08-31T20:29:23Z

@MarkusFrankATcernch Thanks for the additions. This pull request will be very helpful for us in CMS.

MarkusFrankATcernch · 2020-09-01T06:58:35Z

@cvuosalo Yes, but we first have to discuss in the dd4hep meeting the issue of putting the units into a separate namespace - ideally in a user-transparent way.

…ng to reviewers

petricm · 2020-09-04T08:22:01Z

cmake/DD4hepBuild.cmake

@@ -36,7 +36,7 @@ endmacro(dd4hep_to_parent_scope)
 macro(dd4hep_set_compiler_flags)
  include(CheckCXXCompilerFlag)

-  SET(COMPILER_FLAGS -Wall -Wextra -pedantic -Wshadow -Wformat-security -Wno-long-long -Wdeprecated -fdiagnostics-color=auto)
+  SET(COMPILER_FLAGS -Wshadow -Wformat-security -Wno-long-long -Wdeprecated -fdiagnostics-color=auto -Wall -Wextra -pedantic)


What does this re-shuffling change?

Apparently nothing. I had the suspicion that -Wall or -Wextra after -Wshadow disables -Wshadow.
Note that the real order when compiling is inverted.
Net result is that -Wshadow does not work at all for gcc. Only for clang.

MarkusFrankATcernch requested review from andresailer, cvuosalo, gaede, ianna and petricm August 20, 2020 13:13

ianna suggested changes Aug 20, 2020

View reviewed changes

andresailer reviewed Aug 20, 2020

View reviewed changes

cvuosalo reviewed Aug 20, 2020

View reviewed changes

silviodonato mentioned this pull request Aug 26, 2020

Add DDExtrudedPolygon, DDPolycone, and DDPolyhedra shapes for reading shape parameters cms-sw/cmssw#30931

Closed

MarkusFrankATcernch force-pushed the master branch from 20349c8 to 64af59b Compare September 3, 2020 15:09

MarkusFrankATcernch added 12 commits September 3, 2020 17:17

Add basic geometrical accessors to shapes.

81bc9d4

Add basic geometrical accessors to shapes.

a702780

Add basic geometrical accessors to shapes.

ec0494c

Add basic geometrical accessors to shapes.

d03b132

Add basic geometrical accessors to shapes.

b9b3647

Remove shadow warnings

28bd008

Remove shadow warnings

3709fea

Add basic geometrical accessors to shapes. Implement comments accordi…

72b2160

…ng to reviewers

Remove shadow warnings

95bcb21

Remove shadow warnings

eed86d5

Remove shadow warnings

1d21fa3

Remove shadow warnings

0fb059e

Remove shadow warnings

99dea9d

MarkusFrankATcernch force-pushed the master branch from 64af59b to 99dea9d Compare September 4, 2020 08:00

petricm reviewed Sep 4, 2020

View reviewed changes

MarkusFrankATcernch removed the request for review from gaede September 4, 2020 08:42

MarkusFrankATcernch merged commit 8ecaf54 into AIDASoft:master Sep 4, 2020

petricm added the enhancement label Sep 4, 2020

ianna mentioned this pull request Sep 22, 2020

DD4hep update cms-sw/cmsdist#6258

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic geometrical accessors to shapes. #697

Add basic geometrical accessors to shapes. #697

MarkusFrankATcernch commented Aug 20, 2020 •

edited by petricm

Loading

ianna left a comment

ianna Aug 20, 2020

ianna Aug 20, 2020

ianna Aug 20, 2020

MarkusFrankATcernch commented Aug 20, 2020

ianna commented Aug 20, 2020

cvuosalo commented Aug 20, 2020

andresailer left a comment

andresailer Aug 20, 2020

andresailer Aug 20, 2020

andresailer Aug 20, 2020

andresailer Aug 20, 2020

andresailer Aug 20, 2020

andresailer Aug 20, 2020

andresailer Aug 20, 2020

andresailer Aug 20, 2020

cvuosalo commented Aug 20, 2020

cvuosalo Aug 20, 2020

cvuosalo Aug 20, 2020

ianna commented Aug 28, 2020

cvuosalo commented Aug 31, 2020

MarkusFrankATcernch commented Sep 1, 2020

petricm Sep 4, 2020

MarkusFrankATcernch Sep 4, 2020

	/// Angle between centers of x edges an y axis at low z
	/// Angle between centers of x edges and y axis at low z

Add basic geometrical accessors to shapes. #697

Add basic geometrical accessors to shapes. #697

Conversation

MarkusFrankATcernch commented Aug 20, 2020 • edited by petricm Loading

ianna left a 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

MarkusFrankATcernch commented Aug 20, 2020

ianna commented Aug 20, 2020

cvuosalo commented Aug 20, 2020

andresailer left a 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

cvuosalo commented Aug 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianna commented Aug 28, 2020

cvuosalo commented Aug 31, 2020

MarkusFrankATcernch commented Sep 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkusFrankATcernch commented Aug 20, 2020 •

edited by petricm

Loading