Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(geo): CylVolStack reuses gaps if exist #3716

Merged
merged 10 commits into from
Oct 12, 2024
Merged

fix(geo): CylVolStack reuses gaps if exist #3716

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
da0d486
refactor: Add logger to Volume::update
paulgessinger Oct 4, 2024
5bd91eb
refactor: More descriptive CylVolStack overlap printout
paulgessinger Oct 4, 2024
88aaf5a
refactor: Improved output in CylStack, CylBounds, portal links
paulgessinger Oct 4, 2024
d57fc22
fix: CylVolStack resizing issue
paulgessinger Oct 4, 2024
ddb4d46
remove unused header
paulgessinger Oct 11, 2024
b1703d6
Merge branch 'main' into fix/cylvolstack-sizing
kodiakhq[bot] Oct 11, 2024
15241bb
fix(geo): CylVolStack reuses gaps if exist
paulgessinger Oct 8, 2024
9c2b92b
fix(geo): CylVolStack gap reuse comparison bug
paulgessinger Oct 8, 2024
d1d8361
refactor(geo): CylVolStack update output improvements
paulgessinger Oct 8, 2024
8b0788b
add drawing to test for illustration
paulgessinger Oct 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
164 changes: 118 additions & 46 deletions Core/src/Geometry/CylinderVolumeStack.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -754,6 +754,11 @@ void CylinderVolumeStack::update(std::shared_ptr<VolumeBounds> volbounds,
throw std::invalid_argument("Shrinking the stack is r in not supported");
}

auto isGap = [this](const Volume* vol) {
return std::ranges::any_of(
m_gaps, [&](const auto& gap) { return vol == gap.get(); });
};

if (m_direction == BinningValue::binZ) {
ACTS_VERBOSE("Stack direction is z");

Expand Down Expand Up @@ -822,43 +827,86 @@ void CylinderVolumeStack::update(std::shared_ptr<VolumeBounds> volbounds,
} else if (m_resizeStrategy == ResizeStrategy::Gap) {
ACTS_VERBOSE("Creating gap volumes to fill the new z bounds");

if (!same(newMinZ, oldMinZ) && newMinZ < oldMinZ) {
ACTS_VERBOSE("Adding gap volume at negative z");
auto printGapDimensions = [&](const VolumeTuple& gap,
const std::string& prefix = "") {
ACTS_VERBOSE(" -> gap" << prefix << ": [ " << gap.minZ() << " <- "
<< gap.midZ() << " -> " << gap.maxZ()
<< " ], r: [ " << gap.minR() << " <-> "
<< gap.maxR() << " ]");
};

if (!same(newMinZ, oldMinZ) && newMinZ < oldMinZ) {
ActsScalar gap1MinZ = newVolume.minZ();
ActsScalar gap1MaxZ = oldVolume.minZ();
ActsScalar gap1HlZ = (gap1MaxZ - gap1MinZ) / 2.0;
ActsScalar gap1PZ = (gap1MaxZ + gap1MinZ) / 2.0;

ACTS_VERBOSE(" -> gap1 z: [ " << gap1MinZ << " <- " << gap1PZ
<< " -> " << gap1MaxZ
<< " ] (hl: " << gap1HlZ << ")");

auto gap1Bounds =
std::make_shared<CylinderVolumeBounds>(newMinR, newMaxR, gap1HlZ);
auto gap1Transform = m_groupTransform * Translation3{0, 0, gap1PZ};
auto gap1 = addGapVolume(gap1Transform, std::move(gap1Bounds));
volumeTuples.insert(volumeTuples.begin(),
VolumeTuple{*gap1, m_groupTransform});
// // check if we need a new gap volume or reuse an existing one
auto& candidate = volumeTuples.front();
if (isGap(candidate.volume)) {
ACTS_VERBOSE("~> Reusing existing gap volume at negative z");

gap1HlZ =
candidate.bounds->get(CylinderVolumeBounds::eHalfLengthZ) +
gap1HlZ;
gap1MaxZ = gap1MinZ + gap1HlZ * 2;
gap1PZ = (gap1MaxZ + gap1MinZ) / 2.0;

printGapDimensions(candidate, " before");
auto gap1Bounds = std::make_shared<CylinderVolumeBounds>(
newMinR, newMaxR, gap1HlZ);
auto gap1Transform = m_groupTransform * Translation3{0, 0, gap1PZ};
candidate.volume->update(std::move(gap1Bounds), gap1Transform);
candidate = VolumeTuple{*candidate.volume, m_groupTransform};
ACTS_VERBOSE("After:");
printGapDimensions(candidate, " after ");

} else {
ACTS_VERBOSE("~> Creating new gap volume at negative z");
auto gap1Bounds = std::make_shared<CylinderVolumeBounds>(
newMinR, newMaxR, gap1HlZ);
auto gap1Transform = m_groupTransform * Translation3{0, 0, gap1PZ};
auto gap1 = addGapVolume(gap1Transform, std::move(gap1Bounds));
volumeTuples.insert(volumeTuples.begin(),
VolumeTuple{*gap1, m_groupTransform});
printGapDimensions(volumeTuples.front());
}
}

if (!same(newMaxZ, oldMaxZ) && newMaxZ > oldMaxZ) {
ACTS_VERBOSE("Adding gap volume at positive z");

ActsScalar gap2MinZ = oldVolume.maxZ();
ActsScalar gap2MaxZ = newVolume.maxZ();
ActsScalar gap2HlZ = (gap2MaxZ - gap2MinZ) / 2.0;
ActsScalar gap2PZ = (gap2MaxZ + gap2MinZ) / 2.0;

ACTS_VERBOSE(" -> gap2 z: [ " << gap2MinZ << " <- " << gap2PZ
<< " -> " << gap2MaxZ
<< " ] (hl: " << gap2HlZ << ")");

auto gap2Bounds =
std::make_shared<CylinderVolumeBounds>(newMinR, newMaxR, gap2HlZ);
auto gap2Transform = m_groupTransform * Translation3{0, 0, gap2PZ};
auto gap2 = addGapVolume(gap2Transform, std::move(gap2Bounds));
volumeTuples.emplace_back(*gap2, m_groupTransform);
// check if we need a new gap volume or reuse an existing one
auto& candidate = volumeTuples.back();
if (isGap(candidate.volume)) {
ACTS_VERBOSE("~> Reusing existing gap volume at positive z");

gap2HlZ =
candidate.bounds->get(CylinderVolumeBounds::eHalfLengthZ) +
gap2HlZ;
gap2MinZ = newVolume.maxZ() - gap2HlZ * 2;
gap2PZ = (gap2MaxZ + gap2MinZ) / 2.0;

printGapDimensions(candidate, " before");
auto gap2Bounds = std::make_shared<CylinderVolumeBounds>(
newMinR, newMaxR, gap2HlZ);
auto gap2Transform = m_groupTransform * Translation3{0, 0, gap2PZ};

candidate.volume->update(std::move(gap2Bounds), gap2Transform);
candidate = VolumeTuple{*candidate.volume, m_groupTransform};
printGapDimensions(candidate, " after ");
} else {
ACTS_VERBOSE("~> Creating new gap volume at positive z");
auto gap2Bounds = std::make_shared<CylinderVolumeBounds>(
newMinR, newMaxR, gap2HlZ);
auto gap2Transform = m_groupTransform * Translation3{0, 0, gap2PZ};
auto gap2 = addGapVolume(gap2Transform, std::move(gap2Bounds));
volumeTuples.emplace_back(*gap2, m_groupTransform);
printGapDimensions(volumeTuples.back());
}
}
}

Expand Down Expand Up @@ -925,32 +973,56 @@ void CylinderVolumeStack::update(std::shared_ptr<VolumeBounds> volbounds,
<< " ]");
}
} else if (m_resizeStrategy == ResizeStrategy::Gap) {
auto printGapDimensions = [&](const VolumeTuple& gap,
const std::string& prefix = "") {
ACTS_VERBOSE(" -> gap" << prefix << ": [ " << gap.minZ() << " <- "
<< gap.midZ() << " -> " << gap.maxZ()
<< " ], r: [ " << gap.minR() << " <-> "
<< gap.maxR() << " ]");
};

if (oldMinR > newMinR) {
ACTS_VERBOSE("Creating gap volume at the innermost end");
auto gapBounds =
std::make_shared<CylinderVolumeBounds>(newMinR, oldMinR, newHlZ);
auto gapTransform = m_groupTransform;
auto gapVolume = addGapVolume(gapTransform, gapBounds);
volumeTuples.insert(volumeTuples.begin(),
VolumeTuple{*gapVolume, m_groupTransform});
auto gap = volumeTuples.front();
ACTS_VERBOSE(" -> gap inner: [ "
<< gap.minZ() << " <- " << gap.midZ() << " -> "
<< gap.maxZ() << " ], r: [ " << gap.minR() << " <-> "
<< gap.maxR() << " ]");
auto& candidate = volumeTuples.front();
if (isGap(candidate.volume)) {
ACTS_VERBOSE("~> Reusing existing gap volume at inner r");
auto& candidateCylBounds = dynamic_cast<CylinderVolumeBounds&>(
candidate.volume->volumeBounds());
printGapDimensions(candidate, " before");
candidateCylBounds.set(CylinderVolumeBounds::eMinR, newMinR);
candidate = VolumeTuple{*candidate.volume, m_groupTransform};
printGapDimensions(candidate, " after ");
} else {
ACTS_VERBOSE("~> Creating new gap volume at inner r");
auto gapBounds = std::make_shared<CylinderVolumeBounds>(
newMinR, oldMinR, newHlZ);
auto gapTransform = m_groupTransform;
auto gapVolume = addGapVolume(gapTransform, gapBounds);
volumeTuples.insert(volumeTuples.begin(),
VolumeTuple{*gapVolume, m_groupTransform});
auto gap = volumeTuples.front();
printGapDimensions(gap);
}
}
if (oldMaxR < newMaxR) {
ACTS_VERBOSE("Creating gap volume at the outermost end");
auto gapBounds =
std::make_shared<CylinderVolumeBounds>(oldMaxR, newMaxR, newHlZ);
auto gapTransform = m_groupTransform;
auto gapVolume = addGapVolume(gapTransform, gapBounds);
volumeTuples.emplace_back(*gapVolume, m_groupTransform);
auto gap = volumeTuples.back();
ACTS_VERBOSE(" -> gap outer: [ "
<< gap.minZ() << " <- " << gap.midZ() << " -> "
<< gap.maxZ() << " ], r: [ " << gap.minR() << " <-> "
<< gap.maxR() << " ]");
auto& candidate = volumeTuples.back();
if (isGap(candidate.volume)) {
ACTS_VERBOSE("~> Reusing existing gap volume at outer r");
auto& candidateCylBounds = dynamic_cast<CylinderVolumeBounds&>(
candidate.volume->volumeBounds());
printGapDimensions(candidate, " before");
candidateCylBounds.set(CylinderVolumeBounds::eMaxR, newMaxR);
candidate = VolumeTuple{*candidate.volume, m_groupTransform};
printGapDimensions(candidate, " after ");
} else {
ACTS_VERBOSE("~> Creating new gap volume at outer r");
auto gapBounds = std::make_shared<CylinderVolumeBounds>(
oldMaxR, newMaxR, newHlZ);
auto gapTransform = m_groupTransform;
auto gapVolume = addGapVolume(gapTransform, gapBounds);
volumeTuples.emplace_back(*gapVolume, m_groupTransform);
auto gap = volumeTuples.back();
printGapDimensions(gap);
}
}
}

Expand Down
184 changes: 184 additions & 0 deletions Tests/UnitTests/Core/Geometry/CylinderVolumeStackTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -864,6 +864,114 @@ BOOST_AUTO_TEST_CASE(ResizeReproduction2) {
trf3, *logger);
}

// original size
// <--------------->
// +---------------+
// | |
// | |
// | Volume 1 |
// | |
// | |
// +---------------+
// first resize
// <-------------------------->
// +---------------+----------+
// | | |
// | | |
// | Volume 1 | Gap |
// | | | Gap is
// | | | reused!--+
// +---------------+----------+ |
// second resize |
// <-----------------------------------> |
// +---------------+-------------------+ |
// | | | |
// | | | |
// | Volume 1 | Gap |<-----+
// | | |
// | | |
// +---------------+-------------------+
//
BOOST_AUTO_TEST_CASE(ResizeGapMultiple) {
Transform3 trf = Transform3::Identity();
auto bounds = std::make_shared<CylinderVolumeBounds>(70, 100, 100.0);
Volume vol{trf, bounds};

BOOST_TEST_CONTEXT("Positive") {
std::vector<Volume*> volumes = {&vol};
CylinderVolumeStack stack(volumes, BinningValue::binZ,
CylinderVolumeStack::AttachmentStrategy::Gap,
CylinderVolumeStack::ResizeStrategy::Gap,
*logger);

BOOST_CHECK_EQUAL(volumes.size(), 1);
BOOST_CHECK(stack.gaps().empty());

stack.update(std::make_shared<CylinderVolumeBounds>(30.0, 100, 200),
trf * Translation3{Vector3::UnitZ() * 100}, *logger);
BOOST_CHECK_EQUAL(volumes.size(), 2);
BOOST_CHECK_EQUAL(stack.gaps().size(), 1);

BOOST_CHECK_EQUAL(stack.gaps().front()->center()[eZ], 200.0);
const auto* cylBounds = dynamic_cast<const CylinderVolumeBounds*>(
&stack.gaps().front()->volumeBounds());
BOOST_REQUIRE_NE(cylBounds, nullptr);
BOOST_CHECK_EQUAL(cylBounds->get(CylinderVolumeBounds::eHalfLengthZ),
100.0);

stack.update(std::make_shared<CylinderVolumeBounds>(30.0, 100, 300),
trf * Translation3{Vector3::UnitZ() * 200}, *logger);

BOOST_CHECK_EQUAL(volumes.size(), 2);
// No additional gap volume was added!
BOOST_CHECK_EQUAL(stack.gaps().size(), 1);

BOOST_CHECK_EQUAL(stack.gaps().front()->center()[eZ], 300.0);
cylBounds = dynamic_cast<const CylinderVolumeBounds*>(
&stack.gaps().front()->volumeBounds());
BOOST_REQUIRE_NE(cylBounds, nullptr);
BOOST_CHECK_EQUAL(cylBounds->get(CylinderVolumeBounds::eHalfLengthZ),
200.0);
}

BOOST_TEST_CONTEXT("Negative") {
std::vector<Volume*> volumes = {&vol};
CylinderVolumeStack stack(volumes, BinningValue::binZ,
CylinderVolumeStack::AttachmentStrategy::Gap,
CylinderVolumeStack::ResizeStrategy::Gap,
*logger);

BOOST_CHECK_EQUAL(volumes.size(), 1);
BOOST_CHECK(stack.gaps().empty());

stack.update(std::make_shared<CylinderVolumeBounds>(30.0, 100, 200),
trf * Translation3{Vector3::UnitZ() * -100}, *logger);
BOOST_CHECK_EQUAL(volumes.size(), 2);
BOOST_CHECK_EQUAL(stack.gaps().size(), 1);

BOOST_CHECK_EQUAL(stack.gaps().front()->center()[eZ], -200.0);
const auto* cylBounds = dynamic_cast<const CylinderVolumeBounds*>(
&stack.gaps().front()->volumeBounds());
BOOST_REQUIRE_NE(cylBounds, nullptr);
BOOST_CHECK_EQUAL(cylBounds->get(CylinderVolumeBounds::eHalfLengthZ),
100.0);

stack.update(std::make_shared<CylinderVolumeBounds>(30.0, 100, 300),
trf * Translation3{Vector3::UnitZ() * -200}, *logger);

BOOST_CHECK_EQUAL(volumes.size(), 2);
// No additional gap volume was added!
BOOST_CHECK_EQUAL(stack.gaps().size(), 1);

BOOST_CHECK_EQUAL(stack.gaps().front()->center()[eZ], -300.0);
cylBounds = dynamic_cast<const CylinderVolumeBounds*>(
&stack.gaps().front()->volumeBounds());
BOOST_REQUIRE_NE(cylBounds, nullptr);
BOOST_CHECK_EQUAL(cylBounds->get(CylinderVolumeBounds::eHalfLengthZ),
200.0);
}
}

BOOST_AUTO_TEST_SUITE_END()

BOOST_AUTO_TEST_SUITE(RDirection)
Expand Down Expand Up @@ -1554,6 +1662,82 @@ BOOST_DATA_TEST_CASE(
}
}

BOOST_AUTO_TEST_CASE(ResizeGapMultiple) {
Transform3 trf = Transform3::Identity();
auto bounds = std::make_shared<CylinderVolumeBounds>(100, 200, 100);
Volume vol{trf, bounds};

BOOST_TEST_CONTEXT("Outer") {
std::vector<Volume*> volumes = {&vol};
CylinderVolumeStack stack(volumes, BinningValue::binR,
CylinderVolumeStack::AttachmentStrategy::Gap,
CylinderVolumeStack::ResizeStrategy::Gap,
*logger);

BOOST_CHECK_EQUAL(volumes.size(), 1);
BOOST_CHECK(stack.gaps().empty());

stack.update(std::make_shared<CylinderVolumeBounds>(100, 250, 100), trf,
*logger);
BOOST_CHECK_EQUAL(volumes.size(), 2);
BOOST_CHECK_EQUAL(stack.gaps().size(), 1);

const auto* cylBounds = dynamic_cast<const CylinderVolumeBounds*>(
&stack.gaps().front()->volumeBounds());
BOOST_REQUIRE_NE(cylBounds, nullptr);
BOOST_CHECK_EQUAL(cylBounds->get(CylinderVolumeBounds::eMinR), 200);
BOOST_CHECK_EQUAL(cylBounds->get(CylinderVolumeBounds::eMaxR), 250);

stack.update(std::make_shared<CylinderVolumeBounds>(100, 300, 100), trf,
*logger);

BOOST_CHECK_EQUAL(volumes.size(), 2);
// No additional gap volume was added!
BOOST_CHECK_EQUAL(stack.gaps().size(), 1);

cylBounds = dynamic_cast<const CylinderVolumeBounds*>(
&stack.gaps().front()->volumeBounds());
BOOST_REQUIRE_NE(cylBounds, nullptr);
BOOST_CHECK_EQUAL(cylBounds->get(CylinderVolumeBounds::eMinR), 200);
BOOST_CHECK_EQUAL(cylBounds->get(CylinderVolumeBounds::eMaxR), 300);
}

BOOST_TEST_CONTEXT("Inner") {
std::vector<Volume*> volumes = {&vol};
CylinderVolumeStack stack(volumes, BinningValue::binR,
CylinderVolumeStack::AttachmentStrategy::Gap,
CylinderVolumeStack::ResizeStrategy::Gap,
*logger);

BOOST_CHECK_EQUAL(volumes.size(), 1);
BOOST_CHECK(stack.gaps().empty());

stack.update(std::make_shared<CylinderVolumeBounds>(50, 200, 100), trf,
*logger);
BOOST_CHECK_EQUAL(volumes.size(), 2);
BOOST_CHECK_EQUAL(stack.gaps().size(), 1);

const auto* cylBounds = dynamic_cast<const CylinderVolumeBounds*>(
&stack.gaps().front()->volumeBounds());
BOOST_REQUIRE_NE(cylBounds, nullptr);
BOOST_CHECK_EQUAL(cylBounds->get(CylinderVolumeBounds::eMinR), 50);
BOOST_CHECK_EQUAL(cylBounds->get(CylinderVolumeBounds::eMaxR), 100);

stack.update(std::make_shared<CylinderVolumeBounds>(0, 200, 100), trf,
*logger);

BOOST_CHECK_EQUAL(volumes.size(), 2);
// No additional gap volume was added!
BOOST_CHECK_EQUAL(stack.gaps().size(), 1);

cylBounds = dynamic_cast<const CylinderVolumeBounds*>(
&stack.gaps().front()->volumeBounds());
BOOST_REQUIRE_NE(cylBounds, nullptr);
BOOST_CHECK_EQUAL(cylBounds->get(CylinderVolumeBounds::eMinR), 0);
BOOST_CHECK_EQUAL(cylBounds->get(CylinderVolumeBounds::eMaxR), 100);
}
}

BOOST_AUTO_TEST_SUITE_END()

BOOST_AUTO_TEST_SUITE(Common)
Expand Down
Loading