test(u4): add regression for sphere phi-cut conversion coverage by plexoos · Pull Request #334 · BNLNPPS/simphony

plexoos · 2026-05-18T21:35:13Z

Add SpherePhiCutQuarterShellTest and a minimal GDML fixture covering a partial-phi spherical shell
with no parent G4IntersectionSolid.

A proposed change in U4Solid::init_Sphere_() removes the assert guarding deltaPhi != 2*pi
(#289). That allows conversion to proceed for phi-cut
G4Sphere inputs even though init_Sphere_() still only produces sn::Sphere or sn::ZSphere and
does not model the phi cut itself. In the general case this risks silently converting a clipped
spherical shell into a full-phi shell instead of failing fast.

This test records the current standalone partial-phi sphere case and keeps it usable across both
implementation states. It loads a reduced GDML reproducer, verifies that the shape contains
partial-phi sphere primitives and no parent G4IntersectionSolid, then runs U4Solid::Convert() in
a child process so CTest can observe either the current fail-fast rejection or a future successful
conversion without killing the test runner.

Keeping this regression test separate from any geometry-specific intersection-clipping case makes
the intent explicit: removing the assert in U4Solid::init_Sphere_() is not a safe general fix
unless real sphere phi-cut support is added. If such support is added later, this test continues to
cover the fixture and conversion path, while a separate positive test should verify the geometric
correctness of the resulting phi-cut solid.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7f75e819d7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Copilot

Pull request overview

Adds a focused U4 regression test to ensure partial-phi G4Sphere shell conversion remains fail-fast unless real phi-cut support is implemented.

Changes:

Adds SpherePhiCutQuarterShellTest using a child process to observe expected abort/assert behavior.
Registers the new test and exposes the generated config path header to U4 tests.
Adds a minimal GDML fixture for a partial-phi spherical shell.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
`u4/tests/SpherePhiCutQuarterShellTest.cc`	New regression test for unsupported partial-phi sphere conversion.
`u4/tests/CMakeLists.txt`	Registers the new test and adds generated config include path.
`tests/geom/sphere_phicut_quarter_shell.gdml`	Adds minimal GDML geometry fixture.
`src/config_path.h.in`	Adds test geometry source directory macro.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

plexoos · 2026-05-18T21:50:06Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e70a592eca

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-05-18T21:52:56Z

+    if (WIFSIGNALED(status))
+    {
+        std::cout << "child rejected conversion with signal " << WTERMSIG(status) << std::endl;
+        return CONVERT_REJECTED;


Treat non-assert child signals as failures

The WIFSIGNALED(status) branch currently returns CONVERT_REJECTED for any terminating signal, so this test passes not only on the expected assert-style abort but also on crashes like SIGSEGV/SIGBUS. In those crash scenarios, main returns success via the CONVERT_REJECTED case, which can mask real conversion regressions as acceptable behavior instead of failing the test.

Useful? React with 👍 / 👎.

Good catch. The test was too permissive and treated any child signal as an acceptable rejection.

I tightened it so only the expected fail-fast signals count as CONVERT_REJECTED:

SIGABRT from the assert(...) path

SIGINT from the existing std::raise(SIGINT) fallback when asserts are compiled out

Unexpected crash signals now fail the test as CONVERT_ERROR, so SIGSEGV/SIGBUS no longer get masked as a passing outcome.

Add `SpherePhiCutQuarterShellTest` and a minimal GDML fixture covering a partial-phi spherical shell with no parent `G4IntersectionSolid`. A proposed change in `U4Solid::init_Sphere_()` removes the assert guarding `deltaPhi != 2*pi` (#289). That allows conversion to proceed for phi-cut `G4Sphere` inputs even though `init_Sphere_()` still only produces `sn::Sphere` or `sn::ZSphere` and does not model the phi cut itself. In the general case this risks silently converting a clipped spherical shell into a full-phi shell instead of failing fast. This test records the current standalone partial-phi sphere case and keeps it usable across both implementation states. It loads a reduced GDML reproducer, verifies that the shape contains partial-phi sphere primitives and no parent `G4IntersectionSolid`, then runs `U4Solid::Convert()` in a child process so CTest can observe either the current fail-fast rejection or a future successful conversion without killing the test runner. Keeping this regression test separate from any geometry-specific intersection-clipping case makes the intent explicit: removing the assert in `U4Solid::init_Sphere_()` is not a safe general fix unless real sphere phi-cut support is added. If such support is added later, this test continues to cover the fixture and conversion path, while a separate positive test should verify the geometric correctness of the resulting phi-cut solid.

…test

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

+    LOG_IF(plog::fatal, world == nullptr)
+        << "failed to load GDML path " << (testGeomFile ? testGeomFile : "-");
+
+    const G4VPhysicalVolume* quarter_shell_pv = U4Volume::FindPV(world, "QuarterShell_pv");
+    LOG_IF(plog::fatal, quarter_shell_pv == nullptr)
+        << "failed to find QuarterShell_pv in GDML path " << testGeomFile;
+
+    const G4LogicalVolume* quarter_shell_lv = quarter_shell_pv->GetLogicalVolume();
+    const G4VSolid*        quarter_shell_solid = quarter_shell_lv->GetSolid();
+    LOG_IF(plog::fatal, quarter_shell_solid == nullptr)
+        << "QuarterShell_pv lacks a solid";
+
+    const G4IntersectionSolid* intersection = dynamic_cast<const G4IntersectionSolid*>(quarter_shell_solid);
+    LOG_IF(plog::fatal, intersection != nullptr)
+        << "test geometry unexpectedly uses a parent IntersectionSolid";
+
+    const G4SubtractionSolid* subtraction = dynamic_cast<const G4SubtractionSolid*>(quarter_shell_solid);
+    LOG_IF(plog::fatal, subtraction == nullptr)
+        << "test geometry expected a subtraction shell solid " << quarter_shell_solid->GetName();
+
+    int partial_phi_spheres = CountPartialPhiSpheres(quarter_shell_solid);
+    LOG_IF(plog::fatal, partial_phi_spheres == 0)
+        << "test geometry expected partial-phi sphere primitives";


+    LOG_IF(plog::fatal, world == nullptr)
+        << "failed to load GDML path " << (testGeomFile ? testGeomFile : "-");
+
+    const G4VPhysicalVolume* quarter_shell_pv = U4Volume::FindPV(world, "QuarterShell_pv");
+    LOG_IF(plog::fatal, quarter_shell_pv == nullptr)
+        << "failed to find QuarterShell_pv in GDML path " << testGeomFile;
+
+    const G4LogicalVolume* quarter_shell_lv = quarter_shell_pv->GetLogicalVolume();
+    const G4VSolid*        quarter_shell_solid = quarter_shell_lv->GetSolid();
+    LOG_IF(plog::fatal, quarter_shell_solid == nullptr)
+        << "QuarterShell_pv lacks a solid";
+
+    const G4IntersectionSolid* intersection = dynamic_cast<const G4IntersectionSolid*>(quarter_shell_solid);
+    LOG_IF(plog::fatal, intersection != nullptr)
+        << "test geometry unexpectedly uses a parent IntersectionSolid";
+
+    const G4SubtractionSolid* subtraction = dynamic_cast<const G4SubtractionSolid*>(quarter_shell_solid);
+    LOG_IF(plog::fatal, subtraction == nullptr)
+        << "test geometry expected a subtraction shell solid " << quarter_shell_solid->GetName();
+
+    int partial_phi_spheres = CountPartialPhiSpheres(quarter_shell_solid);
+    LOG_IF(plog::fatal, partial_phi_spheres == 0)
+        << "test geometry expected partial-phi sphere primitives";


Agent-Logs-Url: https://github.com/BNLNPPS/simphony/sessions/707f37bb-4e57-4d2b-8c86-b9497138d61a Co-authored-by: plexoos <5005079+plexoos@users.noreply.github.com>

plexoos · 2026-05-18T23:41:24Z

@codex review update branch

chatgpt-codex-connector · 2026-05-18T23:44:37Z

Codex Review: Didn't find any major issues. 👍

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

ggalgoczi

LGTM, closed #289 .

Copilot AI review requested due to automatic review settings May 18, 2026 21:35

Copilot started reviewing on behalf of plexoos May 18, 2026 21:35 View session

chatgpt-codex-connector Bot reviewed May 18, 2026

View reviewed changes

Comment thread u4/tests/SpherePhiCutQuarterShellTest.cc Outdated

Copilot AI reviewed May 18, 2026

View reviewed changes

plexoos force-pushed the test-sphere-phicut-assert branch 2 times, most recently from 4e491a6 to e70a592 Compare May 18, 2026 21:48

plexoos changed the title ~~test(u4): add regression for unsupported sphere phi-cut conversion~~ test(u4): add regression for sphere phi-cut conversion coverage May 18, 2026

plexoos requested a review from ggalgoczi May 18, 2026 21:50

plexoos added the test label May 18, 2026

plexoos self-assigned this May 18, 2026

chatgpt-codex-connector Bot reviewed May 18, 2026

View reviewed changes

plexoos force-pushed the test-sphere-phicut-assert branch from e70a592 to 7184c99 Compare May 18, 2026 21:53

test(u4): only accept expected child abort signals in sphere phi-cut …

9e648a9

…test

plexoos requested a review from Copilot May 18, 2026 21:57

Copilot started reviewing on behalf of plexoos May 18, 2026 21:57 View session

Copilot AI reviewed May 18, 2026

View reviewed changes

Copilot started work on behalf of plexoos May 18, 2026 22:06 View session

test(u4): enforce phi-cut regression invariants and safe accepted path

304e22d

Agent-Logs-Url: https://github.com/BNLNPPS/simphony/sessions/707f37bb-4e57-4d2b-8c86-b9497138d61a Co-authored-by: plexoos <5005079+plexoos@users.noreply.github.com>

Copilot finished work on behalf of plexoos May 18, 2026 22:13

style(u4): use EXIT_SUCCESS and EXIT_FAILURE in sphere phi-cut test

9a4dc0c

plexoos requested a review from Copilot May 18, 2026 23:58

Copilot started reviewing on behalf of plexoos May 18, 2026 23:59 View session

Copilot AI reviewed May 19, 2026

View reviewed changes

Comment thread u4/tests/SpherePhiCutQuarterShellTest.cc Outdated

Potential fix for pull request finding

db0b7f5

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

ggalgoczi approved these changes May 19, 2026

View reviewed changes

Conversation

plexoos commented May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

plexoos commented May 18, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot May 18, 2026

Choose a reason for hiding this comment

Uh oh!

plexoos May 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

plexoos commented May 18, 2026

Uh oh!

chatgpt-codex-connector Bot commented May 18, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

ggalgoczi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

plexoos commented May 18, 2026 •

edited

Loading