Repository navigation
Make mille bug fix vtx - #4498
Open
gregottino wants to merge 4 commits into
Open
gregottino wants to merge 4 commits into
gregottino wants to merge 4 commits into
Conversation
Contributor
Walkthrough
Priority: Merge Risk:
 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
-
⚙️ Run configuration
-
- Source mode: Auto
- Repository: sPHENIX-Collaboration/coresoftware
- Branch: master
- File: .coderabbit.yaml
- Commit: 345cc07
- Layers: Repository YAML
- Review profile: CHILL
- Plan: Advanced
- Run ID:
fb4be642-a802-4116-bd8e-62b12c39e70f
📒 Files selected for processing: 5
-
offline/packages/TrackerMillepedeAlignment/MakeMilleFiles.ccoffline/packages/TrackerMillepedeAlignment/MakeMilleFiles.hoffline/packages/trackbase_historic/SvtxAlignmentState_v1.ccoffline/packages/trackbase_historic/SvtxAlignmentState_v1.hoffline/packages/trackreco/ActsAlignmentStates.cc
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
-
Comment on lines
+806
to
+807
| std::pair<float,float> rotatedVtxCovDiag = getRotatedVtxCovarianceDiagonalPerigee(track, svtxVertex); | ||
| set_vtx_sigma(rotatedVtxCovDiag.first, rotatedVtxCovDiag.second); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
-
🏁 Script executed:
sed -n '760,960p' offline/packages/TrackerMillepedeAlignment/MakeMilleFiles.cc sed -n '1447,1488p' offline/packages/TrackerMillepedeAlignment/MakeMilleFiles.cc rg -n 'sigma <=|sigma >|set_vtx_sigma' offline/packages/TrackerMillepedeAlignment
Repository: sPHENIX-Collaboration/coresoftware
Length of output: 11879
🏁 Script executed:
sed -n '100,125p' offline/packages/TrackerMillepedeAlignment/MakeMilleFiles.h echo '---' sed -n '70,100p' offline/packages/TrackerMillepedeAlignment/Mille.cc echo '---' rg -n -F 'm_vtxSigma' offline/packages/TrackerMillepedeAlignment/MakeMilleFiles.cc offline/packages/TrackerMillepedeAlignment/MakeMilleFiles.h || test "$?" -eq 1
Repository: sPHENIX-Collaboration/coresoftware
Length of output: 3065
Validate the per-track vertex sigma before it reaches Mille.
set_vtx_sigma(rotatedVtxCovDiag.first, rotatedVtxCovDiag.second) replaces m_vtxSigma for every track whose vertex propagation succeeds. The vertex constraint rows and the track_ntp entry for that track then use the replaced value. The per-track sigma is not checked for finiteness or positivity before use. Two failures follow:
- A zero sigma reaches
Mille::mille, which returns early forsigma <= 0.. Both vertex constraint rows for that track are dropped. The cut-flow bin 10 fill runs before these calls, so the track still counts as passed. - A NaN sigma passes the
sigma <= 0.test, because that comparison is false for NaN.Mille::millethen buffers the row for output. A NaN sigma can come from a NaN vertex error. It can also come from a track with zero transverse momentum, becausecotThetadivides bystd::hypot(track->get_px(), track->get_py()). The finite gatehasFiniteVertexResidualchecks the residuals and derivatives, but not the sigma.
Compute the sigma in a local variable and require it to be finite and positive. Otherwise, use the configured m_vtxSigma. Use the local value in the _mille->mille call and in track_ntp.
Proposed fix
-
--- "a/offline/packages/TrackerMillepedeAlignment/MakeMilleFiles.cc" +++ "b/offline/packages/TrackerMillepedeAlignment/MakeMilleFiles.cc" @@ -803,8 +803,13 @@ success2 = getLocalVtxDerivativesXYpsuedo(track, svtxVertex, perigeeCov, lclvtx_derivative_psuedo, localmeaserrpsuedo); - std::pair<float,float> rotatedVtxCovDiag = getRotatedVtxCovarianceDiagonalPerigee(track, svtxVertex); - set_vtx_sigma(rotatedVtxCovDiag.first, rotatedVtxCovDiag.second); + const auto rotatedVtxCovDiag = getRotatedVtxCovarianceDiagonalPerigee(track, svtxVertex); + vtxSigmaTrack = m_vtxSigma; // configured fallback + if (std::isfinite(rotatedVtxCovDiag.first) && rotatedVtxCovDiag.first > 0 && + std::isfinite(rotatedVtxCovDiag.second) && rotatedVtxCovDiag.second > 0) + { + vtxSigmaTrack = {rotatedVtxCovDiag.first, rotatedVtxCovDiag.second}; + } } }
Declare Acts::Vector2 vtxSigmaTrack = m_vtxSigma; before the if (track->size_states() < 2) block. Replace m_vtxSigma(i) with vtxSigmaTrack(i) at lines 882-883 and 932.
Comment on lines
+1191
to
+1195
| const auto intersection = perigee->intersect( | ||
| _tGeometry->geometry().getGeoContext(), | ||
| param.position(_tGeometry->geometry().getGeoContext()), | ||
| param.momentum(), Acts::BoundaryTolerance::None(), | ||
| 0.1 * Acts::UnitConstants::mm).closest(); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
-
🏁 Script executed:
#!/bin/bash rg -n -C3 'perigee->intersect|->intersect\(' --type=cpp offline | head -60
Repository: sPHENIX-Collaboration/coresoftware
Length of output: 1987
🏁 Script executed:
#!/bin/bash set -e printf '%s\n' '--- MakeMilleFiles context ---' nl -ba offline/packages/TrackerMillepedeAlignment/MakeMilleFiles.cc | sed -n '1170,1235p' printf '%s\n' '--- Acts dependency/version references ---' rg -n -F --glob 'CMakeLists.txt' --glob '*.cmake' --glob '*.cmake.in' --glob '*.md' --glob '*.txt' -e 'Acts' -e 'acts' . | head -160 || true printf '%s\n' '--- LineSurface/intersection definitions in checkout ---' rg -n -F --glob '*.{h,hpp,hh,cc,cpp,cxx}' -e 'class LineSurface' -e 'LineSurface::intersect' -e 'intersect(const GeometryContext' -e 'BoundaryTolerance' . | head -240 || true printf '%s\n' '--- Related parameter direction declarations/usages ---' rg -n -F --glob '*.{h,hpp,hh,cc,cpp,cxx}' -e 'param.direction()' -e 'params.direction()' -e 'BoundTrackParameters' offline/packages/trackreco offline/packages/TrackerMillepedeAlignment | head -160 || true
Repository: sPHENIX-Collaboration/coresoftware
Length of output: 7359
🏁 Script executed:
#!/bin/bash set -e printf '%s\n' '--- Acts dependency metadata ---' rg -n -i --glob '!*.root' --glob '!*.o' --glob '!*.so' -e 'find_package[[:space:]]*\([^)]*Acts' -e 'acts(-core)?[[:space:]]*(version|[0-9])' -e 'ACTS_VERSION' -e 'ACTS_ROOT' -e 'Acts_DIR' -e 'acts-core' -e 'Acts::' . | head -240 || true printf '%s\n' '--- Include and factory definitions ---' rg -n -F --glob '*.{h,hpp,cc,cpp,cxx,CMakeLists.txt,cmake}' -e '#include <Acts/' -e 'makeVertexSurface' -e 'makeTrackParams' offline | head -240 || true printf '%s\n' '--- Git submodules and dependency manifests ---' if test -f .gitmodules; then cat .gitmodules; fi find . -maxdepth 3 -type f \( -name '*dependency*' -o -name 'CMakeCache.txt' -o -name 'package.xml' -o -name 'spack.yaml' \) -print | head -100
Repository: sPHENIX-Collaboration/coresoftware
Length of output: 41320
🌐 Web query:
Acts LineSurface intersect implementation normalized direction pathLength cosAlpha axis exact source💡 Result:
<source_evidence> <source kind="open_page"> <title></title> <location>https://raw.githubusercontent.com/acts-project/acts/main/Core/src/Surfaces/LineSurface.cpp</location> <excerpt>citeturn2view0 [wordlim: 200] Crawled: today; Content type: text/plain; Source: open({"ref_id":"https://raw.githubusercontent.com/acts-project/acts/main/Core/src/Surfaces/LineSurface.cpp","lineno":null}); Total lines: 283 L0: // This file is part of the ACTS project. L1: // L2: // Copyright (C) 2016 CERN for the benefit of the ACTS project L3: // L4: // This Source Code Form is subject to the terms of the Mozilla Public L5: // License, v. 2.0. If a copy of the MPL was not distributed with this L6: // file, You can obtain one at https://mozilla.org/MPL/2.0/. L7: L8: #include "Acts/Surfaces/LineSurface.hpp" L9: #include "Acts/Definitions/Algebra.hpp" L10: #include "Acts/Geometry/GeometryObject.hpp" L11: #include "Acts/Surfaces/InfiniteBounds.hpp" L12: #include "Acts/Surfaces/LineBounds.hpp" L13: #include "Acts/Surfaces/SurfaceBounds.hpp" L14: #include "Acts/Surfaces/SurfaceError.hpp" L15: #include "Acts/Surfaces/detail/AlignmentHelper.hpp" L16: #include "Acts/Utilities/AlgebraHelpers.hpp" L17: #include "Acts/Utilities/Intersection.hpp" L18: #include "Acts/Utilities/ThrowAssert.hpp" L19: #include <cmath> L20: #include <limits> L21: #include <utility> L22: L23: namespace Acts { L24: L25: LineSurface::LineSurface(const Transform3& transform, double radius, L26: double halez) L27: : Surface(transform), L28: m_bounds(std::make_shared<const LineBounds>(radius, halez)) {} L29: L30: LineSurface::LineSurface(const Transform3& transform, L31: std::shared_ptr<const LineBounds> lbounds) L32: : Surface(transform), m_bounds(std::move(lbounds)) {} L33: LineSurface::LineSurface(std::shared_ptr<const LineBounds> lbounds, L34: const SurfacePlacementBase& placement) L35: : Surface{placement}, m_bounds(std::move(lbounds)) { L36: throw_assert(m_bounds, "LineBounds must not be nullptr"); L37: } L38: L39: LineSurface::LineSurface(const LineSurface& other) L40: : GeometryObject{}, Surface(other), m_bounds(other.m_bounds) {} L41: LineSurface::LineSurface(const GeometryContext& gctx, const LineSurface& other, L42: const Transform3& shift) L43: : Surface(gctx, other, shift), m_bounds(other.m_bounds) {} L44: L45: LineSurface& LineSurface::operator=(const LineSurface& other) { L46: if (this != &other) { L47: Surface::operator=(other); L48: m_bounds = other.m_bounds; L49: } L50: return *this; L51: } L52: Vector3 LineSurface::localToGlobal(const GeometryContext& gctx, L53: const Vector2& lposition, L54: const Vector3& direction) const { L55: Vector3 unitZ0 = lineDirection(gctx); L56: // get the vector perpendicular to the momentum direction and the straw axis L57: Vector3 radiusAxisGlobal = unitZ0.cross(direction); L58: Vector3 locZinGlobal = L59: localToGlobalTransform(gctx) * Vector3(0., 0., lposition[1]); L60: // add loc0 * radiusAxis L61: return Vector3(locZinGlobal + lposition[0] * radiusAxisGlobal.normalized()); L62: } L63: Result<Vector2> LineSurface::globalToLocal(const GeometryContext& gctx, L64: const Vector3& position, L65: const Vector3& direction, L66: double tolerance) const { L67: using VectorHelpers::perp; L68: // Bring the global position into the local frame. First remove the L69: // translation then the rotation. L70: Vector3 localPosition = L71: referenceFrame(gctx, position, direction).inverse() * L72: (position - localToGlobalTransform(gctx).translation()); L73: // `localPosition.z()` is not …[truncated]</excerpt> </source> <source kind="open_page"> <title>raw.githubusercontent.com</title> <location>https://raw.githubusercontent.com/acts-project/acts/main/Core/include/Acts/Surfaces/LineSurface.hpp</location> <excerpt>citeturn2view1 [wordlim: 200] Crawled: 7 months ago; Content type: text/plain; Source: open({"ref_id":"https://raw.githubusercontent.com/acts-project/acts/main/Core/include/Acts/Surfaces/LineSurface.hpp","lineno":null}); Total lines: 26 L0: // This file is part of the ACTS project. // // Copyright (C) 2016 CERN for the benefit of the ACTS project // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. L1: #pragma once #include "Acts/Definitions/Algebra.hpp" #include "Acts/Definitions/Alignment.hpp" #include "Acts/Definitions/Tolerance.hpp" #include "Acts/Definitions/TrackParametrization.hpp" #include "Acts/Geometry/GeometryContext.hpp" #include "Acts/Surfaces/BoundaryTolerance.hpp" #include "Acts/Surfaces/LineBounds.hpp" #include "Acts/Surfaces/Surface.hpp" #include "Acts/Utilities/AxisDefinitions.hpp" #include "Acts/Utilities/Result.hpp" #include #include namespace Acts { class LineBounds; class SurfaceBounds; /// @class LineSurface /// /// Base class for a linear surfaces in the TrackingGeometry /// to describe dirft tube, straw like detectors or the Perigee /// It inherits from Surface. L2: /// /// @note It leaves the type() method virtual, so it can not be instantiated /// /// @image html LineSurface.png class LineSurface : public Surface { friend class Surface; protected: /// Constructor for LineSurface from Transform3 and radial dimensions /// /// @param transform The transform that positions the line in the global frame /// @param radius The radius of the line /// @param halez The half length in z explicit LineSurface(const Transform3& transform, double radius, double halez); /// Constructor for LineSurface from Transform3 and LineBounds /// /// @param transform The transform that positions the line in the global frame /// @param lbounds The bounds describing the line dimensions explicit LineSurface(const Transform3& transform, std::shared_ptr lbounds = nullptr); /// Constructor from SurfacePlacementBase : Element proxy /// /// @param lbounds are the bounds describing the line dimensions, they must /// not be nullptr /// @param placement Reference to the surface placement /// @note The Surface does not take any ownership over the /// `SurfacePlacementBase` it is expected that the user /// ensures the life-time of the `SurfacePlacementBase` /// and that the `Surface` is actually owned by /// the `SurfacePlacementBase` instance explicit LineSurface(std::shared_ptr lbounds, const SurfacePlacementBase& placement); /// Copy constructor /// /// @param other The source surface for copying LineSurface(const LineSurface& other); /// Copy constructor - with shift /// /// @param gctx The current geometry context object, e.g. L3: alignment /// @param other is the source cone surface /// @param shift is the additional transform applied after copying explicit LineSurface(const GeometryContext& gctx, const LineSurface& other, const Transform3& shift); public: /// Assignment operator /// /// @param other is the source surface dor copying /// @return Reference to this LineSurface after assignment LineSurface& operator=(const LineSurface& other); Vector3 normal(const GeometryContext& gctx, const Vector3& pos, const Vector3& direction) const override; /// The binning position is the position calculated /// for a certain binning type /// /// @param gctx The current geometry context object, e.g. L4: alignment /// @param aDir is the axis direction for the reference position request /// /// @return position that can beused for this binning Vector3 referencePosition(const GeometryContext& gctx, AxisDirection aDir) const final; /// Return the measurement frame - this is needed for alignment, in particu…[truncated]</excerpt> </source> <source kind="search"> <title>ACTS: Acts::LineSurface Class Reference</title> <location>https://acts-project.github.io/class_acts_1_1_line_surface.html</location> <excerpt>citeturn0search0 [wordlim: 200] Crawled: 3 weeks ago; MultiIntersection3D Acts::LineSurface::intersect ( const GeometryContext & gctx, ... expected to be normalized ... The direction vector of the line surface ... std::array< AxisDirection, 2 > Acts::LineSurface::localAxes ( ) const ... * the default implementation is the RotationMatrix3 of the transform Acts::LineSurface Class Reference Base class for a linear surfaces in the TrackingGeometry to describe dirft tube, straw like detectors or the Perigee It inherits from Surface. More... `#include <Acts/Surfaces/LineSurface.hpp>` ## Public Member Functions AlignmentToPathMatrix | alignmentToPathDerivative (const GeometryContext &gctx, const Vector3 &position, const Vector3 &direction) const final | Calculate the derivative of path length at the geometry constraint or point-of-closest-approach w.r.t. void | assignSurfaceBounds (std::shared_ptr< const LineBounds > newBounds) | Overwrite the existing surface bounds with new ones. const SurfaceBounds & | bounds () const final | This method returns the bounds of the surface by reference. const std::shared_ptr< const LineBounds > & | boundsPtr () const | This method returns the shared_ptr to the LineBounds. BoundToFreeMatrix | boundToFreeJacobian (const GeometryContext &gctx, const Vector3 &position, const Vector3 &direction) const final | Calculate the jacobian from local to global which the surface knows best, hence the calculation is done here. FreeToPathMatrix | freeToPathDerivative (const GeometryContext &gctx, const Vector3 &position, const Vector3 &direction) const final | Calculate the derivative of path length at the geometry constraint or point-of-closest-approach w.r.t. Result< Vector2 > | globalToLocal (const GeometryContext &gctx, const Vector3 &position, const Vector3 &direction, double tolerance=s_onSurfaceTolerance) const final | Specified for LineSurface: global to local method without dynamic memory allocation. MultiIntersection3D | intersect (const GeometryContext &gctx, const Vector3 &position, const Vector3 &direction, const BoundaryTolerance &boundaryTolerance=BoundaryTolerance::Infinite(), double tolerance=s_onSurfaceTolerance) const final | Calculate the straight-line intersection with the line surface. Vector3 | lineDirection (const GeometryContext &gctx) const | Get the line direction in global coordinates. Matrix< 2, 3 > | localCartesianToBoundLocalDerivative (const GeometryContext &gctx, const Vector3 &position) const final | Calculate the derivative of bound track parameters local position w.r.t. Vector3 | localToGlobal (const GeometryContext &gctx, const Vector2 &lposition, const Vector3 &direction) const final | Local to global transformation. std::string | name () const override | Return properly formatted class name for screen output. Vector3 | normal (const GeometryContext &gctx, const Vector3 &pos, const Vector3 &direction) const override | Return the surface normal at a given `position` and `direction`. LineSurface & | operator= (const LineSurface &other) | Assignment operator. double | pathCorrection (const GeometryContext &gctx, const Vector3 &position, const Vector3 &direction) const override | the pathCorrection for derived classes with thickness is by definition 1 for LineSurfaces RotationMatrix3 | referenceFrame (const GeometryContext &gctx, const Vector3 &position, const Vector3 &direction) const final | Return the measurement frame - this is needed for alignment, in particular. Vector3 | referencePosition (const GeometryContext &gctx, AxisDirection aDir) const final | The binning position is the position calculated for a certain binning type. Public Member Functions inherited from Acts::Surface Ali…[truncated]</excerpt> </source> <source kind="search"> <title>/master/acts/Core/include/Acts/Surfaces/detail/IntersectionHelper2D.hpp</title> <location>https://sphenix-code-browser.sdcc.bnl.gov/lxr/source/acts/Core/include/Acts/Surfaces/detail/IntersectionHelper2D.hpp</location> <excerpt>citeturn0search1 [wordlim: 200] Crawled: last week; 0001 // This file is part of the ACTS project. ... 0024 /// @param dir The Direction of intersection line ... 0051 static inline std::array<Intersection2D, 2> intersectCircle( 0001 // This file is part of the ACTS project. 0002 // 0003 // Copyright (C) 2016 CERN for the benefit of the ACTS project 0004 // 0005 // This Source Code Form is subject to the terms of the Mozilla Public 0006 // License, v. 2.0. If a copy of the MPL was not distributed with this 0007 // file, You can obtain one at https://mozilla.org/MPL/2.0/. 0008 0009 #pragma once 0010 0011 #include "Acts/Definitions/Algebra.hpp" 0012 #include "Acts/Utilities/Intersection.hpp" 0013 0014 #include <array> 0015 0016 namespace Acts::detail { 0017 0018 struct IntersectionHelper2D { 0019 /// Intersect two segments 0020 /// 0021 /// @param s0 The Start of the segment 0022 /// @param s1 The end of the segment 0023 /// @param origin The Start of intersection line 0024 /// @param dir The Direction of intersection line 0025 /// 0026 /// @return the intersection point with status 0043 0044 /// Intersect the circle 0045 /// 0046 /// @param R The radius 0047 /// @param origin The Start of intersection line 0048 /// @param dir The Direction of intersection line 0049 /// 0050 /// @return the intersection points 0051 static inline std::array<Intersection2D, 2> intersectCircle( 0052 double R, const Vector2& origin, const Vector2& dir) { 0053 return intersectEllipse(R, R, origin, dir); 0054 } 0055 0056 /// Intersect a circle segment 0057 /// 0058 /// @note only forward solution is taken 0059 /// 0060 /// @param R The radius 0061 /// @param phiMin The minimum phi value 0062 /// @param phiMax The maximum phi value 0063 /// @param origin The Start of intersection line 0064 /// @param dir The Direction of intersection line 0065 /// 0066 /// @return the intersection points 0067 static Intersection2D intersectCircleSegment(double R, double phiMin, 0068 double phiMax, 0069 const Vector2& origin, 0070 const Vector2& dir); 0071 0072 }; // struct IntersectionHelper2D 0073 0074 } // namespace Acts::detail</excerpt> </source> <source kind="search"> <title>Development and improvement of track</title> <location>https://openscience.ub.uni-mainz.de/bitstreams/8ee6e375-84e0-4230-8128-33562a158d6d/download</location> <excerpt>citeturn0search12 [wordlim: 200] Published: 6 months ago; Implementation of a fast navigation system through arbitrary geometries ... bool Acts::AxisAlignedBoundingBox<entity_t, value_t, DIM>::intersect( ... the origin point, the frustum direction vector, as well as normal vectors of the sides, the 7.4. Implementation of a fast navigation system through arbitrary geometries template <typename entity_t, typename value_t, size_t DIM> bool Acts::AxisAlignedBoundingBox<entity_t, value_t, DIM>::intersect( const Ray<value_type, DIM>& ray) const { const VertexType& origin = ray.origin(); const vertex_array_type& idir = ray.idir(); // i vertex_array_type t0s = (m_vmin - origin).array() * idir; vertex_array_type t1s = (m_vmax - origin).array() * idir; // ii vertex_array_type tsmaller = t0s.min(t1s); vertex_array_type tbigger = t0s.max(t1s); // iii value_type tmin = tsmaller.maxCoeff(); value_type tmax = tbigger.minCoeff(); /// iv return tmin < tmax && tmax > 0.0; } Listing 7.3.: The algorithm that is implement in ACTS to intersect rays with AABBs. In (i) intersections are calculated, in (ii) the minima and maxima are taken, in (iii) they are combined across dimensions, and in (iv) the result is determined. These calculations are expressed in a way that is uniform across number of dimensions, and the Eigen library can optimize calculations. </excerpt> </source> <omitted_sources count="19">Additional source records were omitted from this bounded result.</omitted_sources> </source_evidence> Citations: - 1: https://raw.githubusercontent.com/acts-project/acts/main/Core/src/Surfaces/LineSurface.cpp - 2: https://raw.githubusercontent.com/acts-project/acts/main/Core/src/Surfaces/LineSurface.cpp - 3: https://raw.githubusercontent.com/acts-project/acts/main/Core/src/Surfaces/LineSurface.cpp - 4: https://raw.githubusercontent.com/acts-project/acts/main/Core/src/Surfaces/LineSurface.cpp
🌐 Web query:
site:github.com/acts-project/acts "LineSurface::intersect" "cosAlpha" "pathLength"💡 Result:
I couldn’t find a GitHub result matching all three terms. The search doesn’t establish whether `LineSurface::intersect` uses `cosAlpha` or `pathLength`; it may be worth searching the repository for each term separately or checking the implementation directly.
🌐 Web query:
site:github.com/acts-project/acts "expected to be normalized" "LineSurface" intersect💡 Result:
I couldn’t find a GitHub result matching that exact phrase. The search doesn’t establish whether the phrase appears elsewhere in the ACTS repository.
🏁 Script executed:
#!/bin/bash set -e python3 - <<'PY' import urllib.request url = "https://raw.githubusercontent.com/acts-project/acts/main/Core/src/Surfaces/LineSurface.cpp" text = urllib.request.urlopen(url, timeout=10).read().decode() lines = text.splitlines() for i, line in enumerate(lines): if "LineSurface::intersect" in line: start = max(0, i - 8) end = min(len(lines), i + 100) for n in range(start, end): print(f"{n+1:4} {lines[n]}") break else: raise SystemExit("intersect definition not found") PY
Repository: sPHENIX-Collaboration/coresoftware
Length of output: 4925
Pass a unit direction to perigee->intersect.
LineSurface::intersect uses the supplied vector directly in its closest-approach calculation and expects a normalized direction. param.momentum() is not normalized. This can produce an invalid or incorrect pathLength(). The code uses that value to choose the propagation direction, so the track can be propagated incorrectly or rejected.
Suggested fix
-
--- "a/offline/packages/TrackerMillepedeAlignment/MakeMilleFiles.cc" +++ "b/offline/packages/TrackerMillepedeAlignment/MakeMilleFiles.cc" @@ -1188,11 +1188,11 @@ ActsPropagator::SphenixPropagator::Options options( _tGeometry->geometry().getGeoContext(), _tGeometry->geometry().magFieldContext); const auto intersection = perigee->intersect( _tGeometry->geometry().getGeoContext(), param.position(_tGeometry->geometry().getGeoContext()), - param.momentum(), Acts::BoundaryTolerance::None(), + param.direction(), Acts::BoundaryTolerance::None(), 0.1 * Acts::UnitConstants::mm).closest(); if (!intersection.isValid()) { if (Verbosity() > 1)
📝 Committable suggestion
-
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.Suggested changeconst auto intersection = perigee->intersect( _tGeometry->geometry().getGeoContext(), param.position(_tGeometry->geometry().getGeoContext()), param.momentum(), Acts::BoundaryTolerance::None(), 0.1 * Acts::UnitConstants::mm).closest(); const auto intersection = perigee->intersect( _tGeometry->geometry().getGeoContext(), param.position(_tGeometry->geometry().getGeoContext()), param.direction(), Acts::BoundaryTolerance::None(), 0.1 * Acts::UnitConstants::mm).closest();
| std::string steering_outfilename = ("steer.txt"); | ||
|
|
||
| bool m_useEventVertex = false; | ||
| bool m_useEventVertex = true; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Changing the m_useEventVertex default to true breaks existing configurations. Add a compatibility note.
Before this change, a macro that did not call use_event_vertex(true) ran without vertex constraints. Now these effects apply by default:
GetNodesreturnsABORTEVENTfromInitRunifSvtxVertexMapis missing. Chains without vertexing therefore stop working.- Every track must have a vertex with a matching crossing and lie within the 2 mm x/y/z window. Otherwise the track is dropped. Track statistics and the Mille output therefore change without notice.
InitRunwrites vertex constraint lines to the constraint file.
The PR description says the change is non-breaking. This default change contradicts that. Do one of these:
- Keep the old default
falseand enable the vertex path explicitly in the alignment macros. - Keep
true, document the change, and update the downstream macros, for example in the macros repository.
Proposed fix (keep previous default)
-
--- "a/offline/packages/TrackerMillepedeAlignment/MakeMilleFiles.h" +++ "b/offline/packages/TrackerMillepedeAlignment/MakeMilleFiles.h" @@ -159,7 +159,7 @@ std::string data_outfilename = ("mille_output_data_file.bin"); std::string steering_outfilename = ("steer.txt"); - bool m_useEventVertex = true; + bool m_useEventVertex = false; bool _binary = true; float m_minPt = 0.0; unsigned int m_minNmvtx = 0;
As per path instructions: "If interfaces change, ask for compatibility notes and any needed downstream updates."
📝 Committable suggestion
-
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.Suggested changebool m_useEventVertex = true; bool m_useEventVertex = false;
Source: Path instructions
Build & test reportReport for commit f7069d40c74a085755cbbe053b820ee3b2faffcb:
Automatically generated by sPHENIX Jenkins continuous integration |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
MakeMilleFiles.cc:1321-1322
offline/packages/TrackerMillepedeAlignment/MakeMilleFiles.cc:1321-1322
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftSubtract the fitted cluster information before generating vertex pseudo-rows.
ActsTransformations::fillSvtxTrackStatesstoresstate.smoothedCovariance()for each fitted cluster measurement.MakeMilleFilespropagates that covariance to the vertex perigee, then uses its full inverse for the vertex pseudo-rows. The same Mille record also receives the cluster rows.The established
ActsAlignmentStates::makeLocalDerivativesPsuedopath computesgetInverseComplement(regularisedCov, weightMatMeasurements)before generating pseudo-rows. Use the corresponding correlation-only complement here. The current direct inverse adds the fitted cluster information again and can over-weight the alignment fit.
MakeMilleFiles.cc:1225-1244
offline/packages/TrackerMillepedeAlignment/MakeMilleFiles.cc:1225-1244
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftUse one common local-parameter reference for every row in a Mille record.
Mille::end()groups the vertex and cluster rows into one set, andMille::mille()assigns each local column the same labels1..NLOC. The base implementation transported the vertex projector withresult.value().transportJacobian. This change replaces that mapping with an identity projector at the vertex perigee. The cluster rows are built directly from each state’s projector on its own reference surface, with no correspondingstate.jacobian()transformation. Mille therefore combines perigeed0/z0derivatives with state-localloc0/loc1derivatives as if they were the same parameters, which can produce an incorrect alignment fit. Restore a single explicit local-parameter reference and transport both vertex and cluster derivatives into that basis.
-
-
- Source mode: Auto
- Repository: sPHENIX-Collaboration/coresoftware
- Branch: master
- File: .coderabbit.yaml
- Commit: 345cc07
- Layers: Repository YAML
- Review profile: CHILL
- Plan: Advanced
- Run ID:
6599afda-0119-4d7f-9850-1e63bccc5ff2
-
offline/packages/TrackerMillepedeAlignment/MakeMilleFiles.cc
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
-
Build & test reportReport for commit bf031055d22b2008aea3273379ba34eea99d74fc:
Automatically generated by sPHENIX Jenkins continuous integration |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Types of changes
What kind of change does this PR introduce? (Bug fix, feature, ...)
comment: Fixes bug in the passing of correlation term in local derivatives for MakeMilleFiles code. Adds functionality to MakeMilleFiles to account for the vertex, and its derivatives (trivial and correlation terms). Creates options to produce diagnostic histgrams when running MakeMille code.
TODOs (if applicable)
Links to other PRs in macros and calibration repositories (if applicable)
Motivation
This change fixes how
MakeMilleFilespasses local correlation terms to Millepede. It also adds support for vertex measurements and their derivatives.Key changes
SvtxAlignmentState_v1. Correct the pseudo-derivative layout and error handling inActsAlignmentStates.Potential risk areas
Possible future improvements
AI-generated summaries can contain errors. Use best judgment and verify details against the implementation.