Skip to content

Commit

Permalink
feat: GeometryId check for Acts::Experimental::Detector (#2870)
Browse files Browse the repository at this point in the history 
Adding GeometryIdentier checks to Acts::Experimental::Detector. Surface Ids are being checked not only for the uniqueness, but also for being assigned in the first place, allowing to avoid undefined GeoId cases. 

Similar checks are added for the DetectorVolume GeoIds. These are, for example, used in the MaterialMapping algo, and the duplicates, as well as undefined Ids, may cause the mapping to malfunction. Using a placeholder unordered_map to check for duplicates.  

In all cases throwing invalid_argument, as it is done in already implemented duplicate check for Surfaces.

Planning to add similar checks for Portals.
  • Loading branch information
@ssdetlab
ssdetlab authored Jan 18, 2024
1 parent fd39512 commit 392612e
Show file tree
Hide file tree
Showing 9 changed files with 168 additions and 4 deletions.
35 changes: 35 additions & 0 deletions Core/src/Detector/Detector.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,9 @@ Acts::Experimental::Detector::Detector(

// Fill the surface map
std::unordered_map<GeometryIdentifier, const Surface*> surfaceGeoIdMap;
// Map for the volume geometry id
std::unordered_map<GeometryIdentifier, const DetectorVolume*> volumeGeoIdMap;

// Check for unique names and fill the volume name / index map
for (auto [iv, v] : enumerate(m_volumes.internal)) {
// Assign this detector
Expand All @@ -67,8 +70,40 @@ Acts::Experimental::Detector::Detector(
}
m_volumeNameIndex[vName] = iv;

// ---------------------------------------------------------------
// Check volume geometry id
auto vgeoID = v->geometryId();
// Check for undefined geometry id
if (vgeoID.value() == 0u) {
throw std::invalid_argument("Detector: volume '" + v->name() +
"' with undefined geometry id detected" +
". Make sure a GeometryIdGenerator is used.");
}
if (volumeGeoIdMap.find(vgeoID) != volumeGeoIdMap.end()) {
std::stringstream ss;
ss << vgeoID;
throw std::invalid_argument("Detector: duplicate volume geometry id '" +
ss.str() + "' detected" +
". Make sure a GeometryIdGenerator is used.");
}
volumeGeoIdMap.emplace(vgeoID, v.get());
// ---------------------------------------------------------------

for (const auto* s : v->surfaces()) {
auto sgeoID = s->geometryId();

// ---------------------------------------------------------------
// Check for undefined geometry id
if (sgeoID.value() == 0u) {
std::stringstream ss;
ss << s->name();
throw std::invalid_argument(
"Detector: surface '" + ss.str() + "' with undefined geometry id " +
"detected in volume '" + v->name() +
"'. Make sure a GeometryIdGenerator is used.");
}
// ---------------------------------------------------------------

if (surfaceGeoIdMap.find(sgeoID) != surfaceGeoIdMap.end()) {
std::stringstream ss;
ss << sgeoID;
Expand Down
11 changes: 9 additions & 2 deletions Plugins/Json/src/DetectorVolumeJsonConverter.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ nlohmann::json Acts::DetectorVolumeJsonConverter::toJson(
const Options& options) {
nlohmann::json jVolume;
jVolume["name"] = volume.name();
jVolume["geometryId"] = volume.geometryId().volume();
jVolume["transform"] = Transform3JsonConverter::toJson(
volume.transform(gctx), options.transformOptions);
jVolume["bounds"] = VolumeBoundsJsonConverter::toJson(volume.volumeBounds());
Expand Down Expand Up @@ -156,6 +157,8 @@ std::shared_ptr<Acts::Experimental::DetectorVolume>
Acts::DetectorVolumeJsonConverter::fromJson(const GeometryContext& gctx,
const nlohmann::json& jVolume) {
std::string name = jVolume["name"];
GeometryIdentifier geoId;
geoId.setVolume(jVolume["geometryId"]);
Transform3 transform =
Transform3JsonConverter::fromJson(jVolume["transform"]);
auto bounds = VolumeBoundsJsonConverter::fromJson(jVolume["bounds"]);
Expand All @@ -167,9 +170,11 @@ Acts::DetectorVolumeJsonConverter::fromJson(const GeometryContext& gctx,
auto portalGenerator = Experimental::defaultPortalGenerator();

if (jSurfaces.empty() && jVolumes.empty()) {
return Experimental::DetectorVolumeFactory::construct(
auto volume = Experimental::DetectorVolumeFactory::construct(
portalGenerator, gctx, name, transform, std::move(bounds),
Experimental::tryAllPortals());
volume->assignGeometryId(geoId);
return volume;
}
// Convert the surfaces
std::vector<std::shared_ptr<Surface>> surfaces;
Expand All @@ -184,8 +189,10 @@ Acts::DetectorVolumeJsonConverter::fromJson(const GeometryContext& gctx,

auto jSurfaceNavigation = jVolume["surface_navigation"];

return Experimental::DetectorVolumeFactory::construct(
auto volume = Experimental::DetectorVolumeFactory::construct(
portalGenerator, gctx, name, transform, std::move(bounds), surfaces,
volumes, Experimental::tryRootVolumes(),
IndexedSurfacesJsonConverter::fromJson(jSurfaceNavigation));
volume->assignGeometryId(geoId);
return volume;
}
41 changes: 41 additions & 0 deletions Tests/UnitTests/Core/Detector/CylindricalDetectorHelperTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "Acts/Detector/Detector.hpp"
#include "Acts/Detector/DetectorComponents.hpp"
#include "Acts/Detector/DetectorVolume.hpp"
#include "Acts/Detector/GeometryIdGenerator.hpp"
#include "Acts/Detector/PortalGenerators.hpp"
#include "Acts/Detector/detail/CylindricalDetectorHelper.hpp"
#include "Acts/Geometry/CuboidVolumeBounds.hpp"
Expand Down Expand Up @@ -149,6 +150,16 @@ BOOST_AUTO_TEST_CASE(ConnectInR) {
BOOST_CHECK_EQUAL(rVolumes[0u]->portalPtrs()[0u], protoContainer[0u]);
BOOST_CHECK_EQUAL(rVolumes[0u]->portalPtrs()[1u], protoContainer[1u]);

// Assign geometry ids to the volumes
Acts::Experimental::GeometryIdGenerator::Config generatorConfig;
GeometryIdGenerator generator(
generatorConfig, Acts::getDefaultLogger("SequentialIdGenerator",
Acts::Logging::VERBOSE));
auto cache = generator.generateCache();
for (auto& vol : rVolumes) {
generator.assignGeometryId(cache, *vol);
}

// A detector construction that should work
auto detector =
Detector::makeShared("DetectorInR", rVolumes, tryRootVolumes());
Expand Down Expand Up @@ -274,6 +285,16 @@ BOOST_AUTO_TEST_CASE(ConnectInZ) {
BOOST_CHECK_EQUAL(protoContainer[ip], zVolumes[0u]->portalPtrs()[ip]);
}

// Assign geometry ids to the volumes
Acts::Experimental::GeometryIdGenerator::Config generatorConfig;
GeometryIdGenerator generator(
generatorConfig, Acts::getDefaultLogger("SequentialIdGenerator",
Acts::Logging::VERBOSE));
auto cache = generator.generateCache();
for (auto& vol : zVolumes) {
generator.assignGeometryId(cache, *vol);
}

auto detector =
Detector::makeShared("DetectorInZ", zVolumes, tryRootVolumes());
}
Expand Down Expand Up @@ -357,6 +378,16 @@ BOOST_AUTO_TEST_CASE(ConnectInPhi) {
}
}

// Assign geometry ids to the volumes
Acts::Experimental::GeometryIdGenerator::Config generatorConfig;
GeometryIdGenerator generator(
generatorConfig, Acts::getDefaultLogger("SequentialIdGenerator",
Acts::Logging::VERBOSE));
auto cache = generator.generateCache();
for (auto& vol : phiVolumes) {
generator.assignGeometryId(cache, *vol);
}

auto detector =
Detector::makeShared("DetectorInPhi", phiVolumes, tryRootVolumes());
}
Expand Down Expand Up @@ -534,6 +565,16 @@ BOOST_AUTO_TEST_CASE(ProtoContainerZR) {
dVolumes.push_back(pecInner);
dVolumes.push_back(pecOuter);

// Assign geometry ids to the volumes
Acts::Experimental::GeometryIdGenerator::Config generatorConfig;
GeometryIdGenerator generator(
generatorConfig, Acts::getDefaultLogger("SequentialIdGenerator",
Acts::Logging::VERBOSE));
auto cache = generator.generateCache();
for (auto& vol : dVolumes) {
generator.assignGeometryId(cache, *vol);
}

auto detector = Detector::makeShared("DetectorFromProtoContainer", dVolumes,
tryRootVolumes());
} // test with different innermost radii
Expand Down
4 changes: 4 additions & 0 deletions Tests/UnitTests/Core/Detector/DetectorBuilderTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,10 @@ class CompBuilder final : public Acts::Experimental::IDetectorComponentBuilder {
portalContainer[ip] = p;
}

Acts::GeometryIdentifier geoID;
geoID.setVolume(1);
dVolume->assignGeometryId(geoID);

return Acts::Experimental::DetectorComponent{
{dVolume},
portalContainer,
Expand Down
21 changes: 21 additions & 0 deletions Tests/UnitTests/Core/Detector/DetectorTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "Acts/Definitions/Algebra.hpp"
#include "Acts/Detector/Detector.hpp"
#include "Acts/Detector/DetectorVolume.hpp"
#include "Acts/Detector/GeometryIdGenerator.hpp"
#include "Acts/Detector/PortalGenerators.hpp"
#include "Acts/Geometry/CylinderVolumeBounds.hpp"
#include "Acts/Geometry/GeometryContext.hpp"
Expand Down Expand Up @@ -87,6 +88,16 @@ BOOST_AUTO_TEST_CASE(DetectorConstruction) {

std::vector<std::shared_ptr<Acts::Experimental::DetectorVolume>> volumes012 =
{cyl0, cyl1, cyl2};

Acts::Experimental::GeometryIdGenerator::Config generatorConfig;
Acts::Experimental::GeometryIdGenerator generator(
generatorConfig,
Acts::getDefaultLogger("SequentialIdGenerator", Acts::Logging::VERBOSE));
auto cache = generator.generateCache();
for (auto& vol : volumes012) {
generator.assignGeometryId(cache, *vol);
}

auto det012 = Acts::Experimental::Detector::makeShared(
"Det012", volumes012, Acts::Experimental::tryRootVolumes());

Expand Down Expand Up @@ -133,6 +144,8 @@ BOOST_AUTO_TEST_CASE(DetectorConstruction) {
std::move(unconnected)),
std::invalid_argument);

generator.assignGeometryId(cache, *cyl0nameDup);

// Misconfigured - duplicate name
std::vector<std::shared_ptr<Acts::Experimental::DetectorVolume>> volumes002 =
{cyl0, cyl0nameDup, cyl2};
Expand Down Expand Up @@ -166,6 +179,14 @@ BOOST_AUTO_TEST_CASE(DetectorConstructionWithHierarchyMap) {
Acts::Experimental::tryNoVolumes(),
Acts::Experimental::tryAllPortalsAndSurfaces());

Acts::Experimental::GeometryIdGenerator::Config generatorConfig;
Acts::Experimental::GeometryIdGenerator generator(
generatorConfig,
Acts::getDefaultLogger("SequentialIdGenerator", Acts::Logging::VERBOSE));

auto cache = generator.generateCache();
generator.assignGeometryId(cache, *cylVolume);

auto det = Acts::Experimental::Detector::makeShared(
"DetWithSurfaces", {cylVolume}, Acts::Experimental::tryRootVolumes());

Expand Down
10 changes: 10 additions & 0 deletions Tests/UnitTests/Core/Detector/IndexedRootVolumeFinderBuilderTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

#include "Acts/Definitions/Algebra.hpp"
#include "Acts/Detector/DetectorVolume.hpp"
#include "Acts/Detector/GeometryIdGenerator.hpp"
#include "Acts/Detector/IndexedRootVolumeFinderBuilder.hpp"
#include "Acts/Detector/PortalGenerators.hpp"
#include "Acts/Geometry/CylinderVolumeBounds.hpp"
Expand Down Expand Up @@ -72,6 +73,15 @@ BOOST_AUTO_TEST_CASE(IndexedRootVolumeFinderBuilderCylindrical) {
// Let's construct a detector
auto rootVolumeFinder = builder.construct(tContext, rootVolumes);

Acts::Experimental::GeometryIdGenerator::Config generatorConfig;
Acts::Experimental::GeometryIdGenerator generator(
generatorConfig,
Acts::getDefaultLogger("SequentialIdGenerator", Acts::Logging::VERBOSE));
auto cache = generator.generateCache();
for (auto& vol : rootVolumes) {
generator.assignGeometryId(cache, *vol);
}

auto detectorIndexed = Detector::makeShared("IndexedDetector", rootVolumes,
std::move(rootVolumeFinder));

Expand Down
8 changes: 8 additions & 0 deletions Tests/UnitTests/Core/Navigation/DetectorNavigatorTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@ BOOST_AUTO_TEST_CASE(DetectorNavigator) {
Acts::Experimental::tryAllSubVolumes(),
Acts::Experimental::tryAllPortalsAndSurfaces());

Acts::GeometryIdentifier innerGeoID;
innerGeoID.setVolume(1);
innerVolume->assignGeometryId(innerGeoID);

auto detectorVolume = Acts::Experimental::DetectorVolumeFactory::construct(
Acts::Experimental::defaultPortalAndSubPortalGenerator(), tgContext,
"Detector Volume", Acts::Transform3::Identity(),
Expand All @@ -66,6 +70,10 @@ BOOST_AUTO_TEST_CASE(DetectorNavigator) {
Acts::Experimental::tryAllSubVolumes(),
Acts::Experimental::tryAllPortalsAndSurfaces());

Acts::GeometryIdentifier geoID;
geoID.setVolume(2);
detectorVolume->assignGeometryId(geoID);

auto detector = Acts::Experimental::Detector::makeShared(
"Detector", {detectorVolume}, Acts::Experimental::tryRootVolumes());

Expand Down
24 changes: 22 additions & 2 deletions Tests/UnitTests/Core/Navigation/DetectorVolumeFindersTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "Acts/Definitions/Algebra.hpp"
#include "Acts/Detector/Detector.hpp"
#include "Acts/Detector/DetectorVolume.hpp"
#include "Acts/Detector/GeometryIdGenerator.hpp"
#include "Acts/Detector/PortalGenerators.hpp"
#include "Acts/Geometry/CylinderVolumeBounds.hpp"
#include "Acts/Geometry/GeometryContext.hpp"
Expand Down Expand Up @@ -66,13 +67,24 @@ auto cyl2 = Acts::Experimental::DetectorVolumeFactory::construct(

std::vector<std::shared_ptr<Acts::Experimental::DetectorVolume>> volumes012 = {
cyl0, cyl1, cyl2};
auto det012 = Acts::Experimental::Detector::makeShared(
"Det012", volumes012, Acts::Experimental::tryRootVolumes());

Acts::Experimental::GeometryIdGenerator::Config generatorConfig;
Acts::Experimental::GeometryIdGenerator generator(
generatorConfig,
Acts::getDefaultLogger("SequentialIdGenerator", Acts::Logging::VERBOSE));

BOOST_AUTO_TEST_SUITE(Experimental)

// Test finding detectors by trial and error
BOOST_AUTO_TEST_CASE(RootVolumeFinder) {
auto cache = generator.generateCache();
for (auto& vol : volumes012) {
generator.assignGeometryId(cache, *vol);
}

auto det012 = Acts::Experimental::Detector::makeShared(
"Det012", volumes012, Acts::Experimental::tryRootVolumes());

nState.currentDetector = det012.get();
Acts::Experimental::RootVolumeFinder rvf;
// Cylinder 0
Expand All @@ -94,6 +106,14 @@ BOOST_AUTO_TEST_CASE(RootVolumeFinder) {

// Test finding detectors beu trial and error
BOOST_AUTO_TEST_CASE(IndexedDetectorVolumeFinder) {
auto cache = generator.generateCache();
for (auto& vol : volumes012) {
generator.assignGeometryId(cache, *vol);
}

auto det012 = Acts::Experimental::Detector::makeShared(
"Det012", volumes012, Acts::Experimental::tryRootVolumes());

nState.currentDetector = det012.get();

using SingleIndex = std::size_t;
Expand Down
18 changes: 18 additions & 0 deletions Tests/UnitTests/Plugins/Json/DetectorJsonConverterTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,15 @@ BOOST_AUTO_TEST_CASE(SingleEmptyVolumeDetector) {
std::vector<std::shared_ptr<Acts::Experimental::DetectorVolume>> volumes = {
volume};

Acts::Experimental::GeometryIdGenerator::Config generatorConfig;
Acts::Experimental::GeometryIdGenerator generator(
generatorConfig,
Acts::getDefaultLogger("SequentialIdGenerator", Acts::Logging::VERBOSE));
auto cache = generator.generateCache();
for (auto& vol : volumes) {
generator.assignGeometryId(cache, *vol);
}

auto detector = Acts::Experimental::Detector::makeShared(
"Detector", volumes, Acts::Experimental::tryRootVolumes());

Expand Down Expand Up @@ -114,6 +123,15 @@ BOOST_AUTO_TEST_CASE(SingleVolumeOneSurfaceDetector) {
std::vector<std::shared_ptr<Acts::Experimental::DetectorVolume>> volumes = {
volume};

Acts::Experimental::GeometryIdGenerator::Config generatorConfig;
Acts::Experimental::GeometryIdGenerator generator(
generatorConfig,
Acts::getDefaultLogger("SequentialIdGenerator", Acts::Logging::VERBOSE));
auto cache = generator.generateCache();
for (auto& vol : volumes) {
generator.assignGeometryId(cache, *vol);
}

auto detector = Acts::Experimental::Detector::makeShared(
"Detector", volumes, Acts::Experimental::tryRootVolumes());

Expand Down

0 comments on commit 392612e

Please sign in to comment.