Allow for restricted cos(theta_z) in DIO generation

[michaelmackenzie]

No description provided.

[FNALbuild]

Hi @michaelmackenzie,
You have proposed changes to files in these packages:

• EventGenerator

which require these tests: build.

@Mu2e/fnalbuild-users, @Mu2e/write have access to CI actions on main.

⌛ The following tests have been triggered for dc4139c: build (Build queue - API unavailable)

About FNALbuild. Code review on Mu2e/Offline.

[FNALbuild]

☀️ The build tests passed at dc4139c.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged dc4139c at db6161d
build (prof)	✅	Log file. Build time: 08 min 44 sec
ceSimReco	✅	Log file.
g4test_03MT	✅	Log file.
transportOnly	✅	Log file.
POT	✅	Log file.
g4study	✅	Log file.
cosmicSimReco	✅	Log file.
cosmicOffSpill	✅	Log file.
ceSteps	✅	Log file.
ceDigi	✅	Log file.
muDauSteps	✅	Log file.
ceMix	✅	Log file.
rootOverlaps	✅	Log file.
g4surfaceCheck	✅	Log file.
trigger	✅	Log file.
check_cmake	✅	Log file.
FIXME, TODO	✅	TODO (0) FIXME (0) in 1 files
clang-tidy	➡️	2 errors 7 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at dc4139c after being merged into the base branch at db6161d.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

[oksuzian]

I'll review this pull request by first loading the PR details and its changes.Now let me look at the full file and the RandomUnitSphere class to understand the broader context:Now I have a complete picture of the PR. Let me also check the RMCGenerator_tool.cc to see how it handles the same pattern (for comparison with validation logic):Here is my review of PR #1730:

Summary

This PR adds two new FHiCL configuration parameters (czmin, czmax) to DIOGenerator to allow restricting the cos(θ_z) range when generating DIO (Decay-In-Orbit) events, mirroring the same feature that already exists in RMCGenerator_tool.cc.

The change is small (6 additions, 1 deletion) and modifies only one file: EventGenerator/src/DIOGenerator_tool.cc.

---

Issues Found

1. 🔴 Missing input validation for czmin/czmax

The RMCGenerator (which this PR follows as a pattern) validates the cos(θ_z) range in its constructor:

if(_czmin > _czmax || _czmin < -1. || _czmax > 1.) throw cet::exception("BADCONFIG") << "RMCGenerator cos(theta_z) range is not defined\n";

The DIOGenerator in this PR does not validate the new czmin/czmax inputs at all. Invalid configurations such as czmin > czmax, czmin < -1, or czmax > 1 would be silently accepted. While the fhicl::Atom defaults are valid (-1. and 1.), user-supplied values could be nonsensical and would produce incorrect physics results without any error message.

Recommendation: Add a validation check in the DIOGenerator constructor body, similar to RMCGenerator:

if(_czmin > _czmax || _czmin < -1. || _czmax > 1.)
  throw cet::exception("BADCONFIG") << "DIOGenerator cos(theta_z) range is not defined\n";

2. 🟡 No accounting for the angular restriction in the normalization

The constructor already computes a "sampled spectrum fraction" to account for the restricted energy range, printing lines like:

std::cout << "Sampled spectrum fraction " << integral/fullintegral << std::endl;

However, restricting cos(��_z) also reduces the solid angle coverage. The fraction of the unit sphere sampled is (czmax - czmin) / 2. This geometric factor is not included in the printed "sampled spectrum fraction." If downstream code relies on this fraction to correctly weight the DIO rate, the normalization will be wrong when czmin/czmax are restricted.

Recommendation: Either:

• Multiply the printed fraction by (czmax - czmin) / 2.0 so the total efficiency accounts for both the energy and angular restrictions, or
• Print the angular fraction separately so users are aware.

3. 🟡 Declaration style: two variables on one line

double _czmin, _czmax;

Declaring multiple member variables on a single line can reduce readability and makes diffs less precise. The RMCGenerator declares them on separate lines:

    const double _czmin; //range of cos(theta_z) generated
    const double _czmax;

Recommendation: Declare on separate lines and consider adding comments and/or making them const, consistent with RMCGenerator.

4. 🟢 Minor: Missing const qualifier on _czmin and _czmax

In RMCGenerator, the equivalent members are declared const:

    const double _czmin; //range of cos(theta_z) generated
    const double _czmax;

Since these values are set in the initializer list and never modified, they should be const for consistency and safety.

---

What Looks Good

• ✅ The RandomUnitSphere class already supports czmin/czmax parameters with appropriate defaults, so the API usage is correct.
• ✅ FHiCL parameters have sensible defaults (-1. and 1.) that preserve backward compatibility — existing configurations are unaffected.
• ✅ The pattern follows RMCGenerator_tool.cc (authored by the same developer), showing consistency in approach.
• ✅ Member initialization order in the constructor matches the declaration order (after fixing), avoiding undefined behavior.

[FNALbuild]

📝 The HEAD of main has changed to 10120d9. Tests are now out of date.

[michaelmackenzie]

@FNALbuild run build test

[FNALbuild]

⌛ The following tests have been triggered for d8210c7: build (Build queue - API unavailable)

[FNALbuild]

☔ The build is failing at d8210c7.

scons: *** [build/al9-prof-e29-p092/Offline/tmp/EventGenerator/src/DIOGenerator_tool.os] Error 1

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged d8210c7 at 35b6176
build (prof)	❌	Log file.
ceSimReco	〰️	Log file.
g4test_03MT	〰️	Log file.
transportOnly	〰️	Log file.
POT	〰️	Log file.
g4study	〰️	Log file.
cosmicSimReco	〰️	Log file.
cosmicOffSpill	〰️	Log file.
ceSteps	〰️	Log file.
ceDigi	〰️	Log file.
muDauSteps	〰️	Log file.
ceMix	〰️	Log file.
rootOverlaps	〰️	Log file.
g4surfaceCheck	〰️	Log file.
trigger	❗	Log file.
check_cmake	〰️	Log file.
FIXME, TODO	✅	TODO (0) FIXME (0) in 1 files
clang-tidy	➡️	3 errors 12 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at d8210c7 after being merged into the base branch at 35b6176.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

[michaelmackenzie]

@FNALbuild run build test

[FNALbuild]

⌛ The following tests have been triggered for 331511e: build (Build queue - API unavailable)

[FNALbuild]

☀️ The build tests passed at 331511e.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 331511e at 35b6176
build (prof)	✅	Log file. Build time: 04 min 15 sec
ceSimReco	✅	Log file.
g4test_03MT	✅	Log file.
transportOnly	✅	Log file.
POT	✅	Log file.
g4study	✅	Log file.
cosmicSimReco	✅	Log file.
cosmicOffSpill	✅	Log file.
ceSteps	✅	Log file.
ceDigi	✅	Log file.
muDauSteps	✅	Log file.
ceMix	✅	Log file.
rootOverlaps	✅	Log file.
g4surfaceCheck	✅	Log file.
trigger	✅	Log file.
check_cmake	✅	Log file.
FIXME, TODO	✅	TODO (0) FIXME (0) in 1 files
clang-tidy	➡️	2 errors 14 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 331511e after being merged into the base branch at 35b6176.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.