Skip to content

Commit

Permalink
chore: Cleanup some surface code (#3991)
Browse files Browse the repository at this point in the history
- clean includes
- remove explicitly deleted or defaulted constructors / destructors
  - allows c++ to generate move constructor / assignment implicitly
- clean namespace in source files

<!-- This is an auto-generated comment: release notes by coderabbit.ai -->
## Summary by CodeRabbit

- **New Features**
	- Added `values()` method to multiple classes (e.g., `AnnulusBounds`, `ConeBounds`, `DiamondBounds`, `DiscTrapezoidBounds`, `EllipseBounds`, `RadialBounds`, `RectangleBounds`, `TrapezoidVolumeBounds`) to return internal state as a vector of doubles.
	- Introduced `checkConsistency()` method in several classes to validate parameters and throw exceptions for invalid configurations.

- **Bug Fixes**
	- Enhanced error handling in `checkConsistency()` methods across various classes to throw `std::invalid_argument` exceptions for invalid inputs.

- **Refactor**
	- Removed default constructors and destructors from multiple classes to enforce parameterized construction.
	- Streamlined method implementations by removing unnecessary namespace prefixes and inline definitions.

- **Documentation**
	- Improved comments and documentation within the code to clarify functionality and usage.

- **Tests**
	- Expanded unit tests for various classes to ensure comprehensive coverage of functionality and error handling.

- **Chores**
	- Removed unused include directives from several files to clean up dependencies.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
  • Loading branch information
andiwand authored Dec 19, 2024
1 parent 32d4bc1 commit 72145fc
Show file tree
Hide file tree
Showing 77 changed files with 881 additions and 1,300 deletions.
25 changes: 0 additions & 25 deletions Core/include/Acts/Geometry/CutoutCylinderVolumeBounds.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
#include <array>
#include <iosfwd>
#include <memory>
#include <stdexcept>
#include <vector>

namespace Acts {
Expand Down Expand Up @@ -47,8 +46,6 @@ class CutoutCylinderVolumeBounds : public VolumeBounds {
eSize
};

CutoutCylinderVolumeBounds() = delete;

/// Constructor from defining parameters
///
/// @param rmin Minimum radius at the "choke points"
Expand All @@ -73,8 +70,6 @@ class CutoutCylinderVolumeBounds : public VolumeBounds {
buildSurfaceBounds();
}

~CutoutCylinderVolumeBounds() override = default;

VolumeBounds::BoundsType type() const final {
return VolumeBounds::eCutoutCylinder;
}
Expand Down Expand Up @@ -151,24 +146,4 @@ class CutoutCylinderVolumeBounds : public VolumeBounds {
void checkConsistency() noexcept(false);
};

inline std::vector<double> CutoutCylinderVolumeBounds::values() const {
std::vector<double> valvector;
valvector.insert(valvector.begin(), m_values.begin(), m_values.end());
return valvector;
}

inline void CutoutCylinderVolumeBounds::checkConsistency() noexcept(false) {
if (get(eMinR) < 0. || get(eMedR) <= 0. || get(eMaxR) <= 0. ||
get(eMinR) >= get(eMedR) || get(eMinR) >= get(eMaxR) ||
get(eMedR) >= get(eMaxR)) {
throw std::invalid_argument(
"CutoutCylinderVolumeBounds: invalid radial input.");
}
if (get(eHalfLengthZ) <= 0 || get(eHalfLengthZcutout) <= 0. ||
get(eHalfLengthZcutout) > get(eHalfLengthZ)) {
throw std::invalid_argument(
"CutoutCylinderVolumeBounds: invalid longitudinal input.");
}
}

} // namespace Acts
95 changes: 14 additions & 81 deletions Core/include/Acts/Surfaces/AnnulusBounds.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,14 @@

#include "Acts/Definitions/Algebra.hpp"
#include "Acts/Definitions/Tolerance.hpp"
#include "Acts/Definitions/TrackParametrization.hpp"
#include "Acts/Surfaces/BoundaryTolerance.hpp"
#include "Acts/Surfaces/DiscBounds.hpp"
#include "Acts/Surfaces/SurfaceBounds.hpp"
#include "Acts/Utilities/detail/periodic.hpp"

#include <array>
#include <cmath>
#include <exception>
#include <iosfwd>
#include <numbers>
#include <stdexcept>
#include <vector>

namespace Acts {
Expand Down Expand Up @@ -49,8 +45,6 @@ class AnnulusBounds : public DiscBounds {
eSize = 7
};

AnnulusBounds() = delete;

/// @brief Default constructor from parameters
/// @param minR inner radius, in module system
/// @param maxR outer radius, in module system
Expand All @@ -74,7 +68,7 @@ class AnnulusBounds : public DiscBounds {

AnnulusBounds(const AnnulusBounds& source) = default;

SurfaceBounds::BoundsType type() const final;
BoundsType type() const final { return SurfaceBounds::eAnnulus; }

/// Return the bound values as dynamically sized vector
///
Expand Down Expand Up @@ -102,24 +96,29 @@ class AnnulusBounds : public DiscBounds {

/// @brief Returns the right angular edge of the module
/// @return The right side angle
double phiMin() const;
double phiMin() const { return get(eMinPhiRel) + get(eAveragePhi); }

/// @brief Returns the left angular edge of the module
/// @return The left side angle
double phiMax() const;
double phiMax() const { return get(eMaxPhiRel) + get(eAveragePhi); }

/// Returns true for full phi coverage
bool coversFullAzimuth() const final;
bool coversFullAzimuth() const final {
return (std::abs((get(eMinPhiRel) - get(eMaxPhiRel)) - std::numbers::pi) <
s_onSurfaceTolerance);
}

/// Checks if this is inside the radial coverage
/// given the a tolerance
bool insideRadialBounds(double R, double tolerance = 0.) const final;
bool insideRadialBounds(double R, double tolerance = 0.) const final {
return ((R + tolerance) > get(eMinR) && (R - tolerance) < get(eMaxR));
}

/// Return a reference radius for binning
double binningValueR() const final;
double binningValueR() const final { return 0.5 * (get(eMinR) + get(eMaxR)); }

/// Return a reference radius for binning
double binningValuePhi() const final;
double binningValuePhi() const final { return get(eAveragePhi); }

/// @brief Returns moduleOrigin, but rotated out, so @c averagePhi is already
/// considered. The module origin needs to consider the rotation introduced by
Expand Down Expand Up @@ -150,10 +149,10 @@ class AnnulusBounds : public DiscBounds {
unsigned int quarterSegments = 2u) const override;

/// This method returns inner radius
double rMin() const final;
double rMin() const final { return get(eMinR); }

/// This method returns outer radius
double rMax() const final;
double rMax() const final { return get(eMaxR); }

private:
std::array<double, eSize> m_values;
Expand Down Expand Up @@ -202,72 +201,6 @@ class AnnulusBounds : public DiscBounds {
/// @param vStripXY the position in the cartesian strip system
/// @return the position in the module polar coordinate system
Vector2 stripXYToModulePC(const Vector2& vStripXY) const;

/// Private helper method
Vector2 closestOnSegment(const Vector2& a, const Vector2& b, const Vector2& p,
const SquareMatrix2& weight) const;

/// Private helper method
double squaredNorm(const Vector2& v, const SquareMatrix2& weight) const;
};

inline SurfaceBounds::BoundsType AnnulusBounds::type() const {
return SurfaceBounds::eAnnulus;
}

inline double AnnulusBounds::rMin() const {
return get(eMinR);
}

inline double AnnulusBounds::rMax() const {
return get(eMaxR);
}

inline double AnnulusBounds::phiMin() const {
return get(eMinPhiRel) + get(eAveragePhi);
}

inline double AnnulusBounds::phiMax() const {
return get(eMaxPhiRel) + get(eAveragePhi);
}

inline bool AnnulusBounds::coversFullAzimuth() const {
return (std::abs((get(eMinPhiRel) - get(eMaxPhiRel)) - std::numbers::pi) <
s_onSurfaceTolerance);
}

inline bool AnnulusBounds::insideRadialBounds(double R,
double tolerance) const {
return ((R + tolerance) > get(eMinR) && (R - tolerance) < get(eMaxR));
}

inline double AnnulusBounds::binningValueR() const {
return 0.5 * (get(eMinR) + get(eMaxR));
}

inline double AnnulusBounds::binningValuePhi() const {
return get(eAveragePhi);
}

inline std::vector<double> AnnulusBounds::values() const {
std::vector<double> valvector;
valvector.insert(valvector.begin(), m_values.begin(), m_values.end());
return valvector;
}

inline void AnnulusBounds::checkConsistency() noexcept(false) {
if (get(eMinR) < 0. || get(eMaxR) < 0. || get(eMinR) > get(eMaxR) ||
std::abs(get(eMinR) - get(eMaxR)) < s_epsilon) {
throw std::invalid_argument("AnnulusBounds: invalid radial setup.");
}
if (get(eMinPhiRel) != detail::radian_sym(get(eMinPhiRel)) ||
get(eMaxPhiRel) != detail::radian_sym(get(eMaxPhiRel)) ||
get(eMinPhiRel) > get(eMaxPhiRel)) {
throw std::invalid_argument("AnnulusBounds: invalid phi boundary setup.");
}
if (get(eAveragePhi) != detail::radian_sym(get(eAveragePhi))) {
throw std::invalid_argument("AnnulusBounds: invalid phi positioning.");
}
}

} // namespace Acts
3 changes: 0 additions & 3 deletions Core/include/Acts/Surfaces/BoundaryTolerance.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,8 @@

#include "Acts/Definitions/Algebra.hpp"

#include <cmath>
#include <iterator>
#include <optional>
#include <variant>
#include <vector>

namespace Acts {

Expand Down
46 changes: 5 additions & 41 deletions Core/include/Acts/Surfaces/ConeBounds.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,14 @@
#pragma once

#include "Acts/Definitions/Algebra.hpp"
#include "Acts/Definitions/Tolerance.hpp"
#include "Acts/Surfaces/BoundaryTolerance.hpp"
#include "Acts/Surfaces/SurfaceBounds.hpp"
#include "Acts/Utilities/detail/periodic.hpp"

#include <array>
#include <cmath>
#include <cstdlib>
#include <iosfwd>
#include <numbers>
#include <stdexcept>
#include <vector>

namespace Acts {
Expand All @@ -34,7 +31,6 @@ namespace Acts {
///
/// @image html ConeBounds.gif
///

class ConeBounds : public SurfaceBounds {
public:
enum BoundValues : int {
Expand All @@ -46,8 +42,6 @@ class ConeBounds : public SurfaceBounds {
eSize = 5
};

ConeBounds() = delete;

/// Constructor - open cone with alpha, by default a full cone
/// but optionally can make a conical section
///
Expand Down Expand Up @@ -77,9 +71,7 @@ class ConeBounds : public SurfaceBounds {
/// @param values The parameter array
ConeBounds(const std::array<double, eSize>& values) noexcept(false);

~ConeBounds() override = default;

BoundsType type() const final;
BoundsType type() const final { return SurfaceBounds::eCone; }

/// Return the bound values as dynamically sized vector
///
Expand All @@ -105,10 +97,10 @@ class ConeBounds : public SurfaceBounds {
///
/// @param z is the z value for which r is requested
/// @return is the r value associated with z
double r(double z) const;
double r(double z) const { return std::abs(z * m_tanAlpha); }

/// Return tangent of alpha (pre-computed)
double tanAlpha() const;
double tanAlpha() const { return m_tanAlpha; }

/// Access to the bound values
/// @param bValue the class nested enum for the array access
Expand All @@ -124,38 +116,10 @@ class ConeBounds : public SurfaceBounds {

/// Private helper function to shift a local 2D position
///
/// Shift r-phi coordinate to be centered around the average phi.
///
/// @param lposition The original local position
Vector2 shifted(const Vector2& lposition) const;
};

inline double ConeBounds::r(double z) const {
return std::abs(z * m_tanAlpha);
}

inline double ConeBounds::tanAlpha() const {
return m_tanAlpha;
}

inline std::vector<double> ConeBounds::values() const {
std::vector<double> valvector;
valvector.insert(valvector.begin(), m_values.begin(), m_values.end());
return valvector;
}

inline void ConeBounds::checkConsistency() noexcept(false) {
if (get(eAlpha) < 0. || get(eAlpha) >= std::numbers::pi) {
throw std::invalid_argument("ConeBounds: invalid open angle.");
}
if (get(eMinZ) > get(eMaxZ) ||
std::abs(get(eMinZ) - get(eMaxZ)) < s_epsilon) {
throw std::invalid_argument("ConeBounds: invalid z range setup.");
}
if (get(eHalfPhiSector) < 0. || abs(eHalfPhiSector) > std::numbers::pi) {
throw std::invalid_argument("ConeBounds: invalid phi sector setup.");
}
if (get(eAveragePhi) != detail::radian_sym(get(eAveragePhi))) {
throw std::invalid_argument("ConeBounds: invalid phi positioning.");
}
}

} // namespace Acts
8 changes: 1 addition & 7 deletions Core/include/Acts/Surfaces/ConeSurface.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
#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/Geometry/Polyhedron.hpp"
#include "Acts/Surfaces/BoundaryTolerance.hpp"
Expand All @@ -23,8 +22,6 @@
#include "Acts/Utilities/Result.hpp"
#include "Acts/Utilities/detail/RealQuadraticEquation.hpp"

#include <cmath>
#include <cstddef>
#include <memory>
#include <numbers>
#include <string>
Expand All @@ -41,7 +38,7 @@ namespace Acts {
/// at the tip of the cone.
/// Propagations to a cone surface will be returned in
/// curvilinear coordinates.

///
class ConeSurface : public RegularSurface {
friend class Surface;

Expand Down Expand Up @@ -85,9 +82,6 @@ class ConeSurface : public RegularSurface {
const Transform3& shift);

public:
~ConeSurface() override = default;
ConeSurface() = delete;

/// Assignment operator
///
/// @param other is the source surface for the assignment
Expand Down
Loading

0 comments on commit 72145fc

Please sign in to comment.