Allow cz restriction with CE generation by michaelmackenzie · Pull Request #1782 · Mu2e/Offline

michaelmackenzie · 2026-04-06T15:16:03Z

Allow cos(theta_z) restriction in Ce endpoint random unit sphere

FNALbuild · 2026-04-06T15:16:30Z

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

EventGenerator

which require these tests: build.

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

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

About FNALbuild. Code review on Mu2e/Offline.

FNALbuild · 2026-04-06T15:28:35Z

☀️ The build tests passed at c54976b.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged `c54976b` at `a31b07e`
build (prof)	✅	Log file. Build time: 04 min 07 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 5 warnings
whitespace check	✅	no whitespace errors found

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

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 · 2026-04-06T15:30:26Z

I'll review this pull request by examining the changes and details.Now let me look at the RandomUnitSphere class to understand its constructor and how czMin/czMax are handled:Here's my review of PR #1782. The change is small (1 file, +3/−1 lines) and the core idea — exposing czMin/czMax fhicl parameters and forwarding them to RandomUnitSphere — is straightforward and correct. However, I found several issues worth addressing:

1. 🔴 No input validation on `czMin` / `czMax`

There is no check that the user-provided values are physically sensible. Invalid configurations will silently produce wrong results (or no results at all if czMin > czMax).

Suggested fix — add validation in the constructor body (around line 90):

    , randomUnitSphere_{eng_, conf().czMin(), conf().czMax()}
    , pdgId_(conf().pdgId())

Add after line 90 inside the constructor body:

    if(conf().czMin() < -1. || conf().czMax() > 1. || conf().czMin() >= conf().czMax()) {
      throw cet::exception("BADINPUT")
        << "CeEndpoint: invalid czMin/czMax range ["
        << conf().czMin() << ", " << conf().czMax() << "]. "
        << "Require -1 <= czMin < czMax <= 1.\n";
    }

2. 🟡 Missing normalization / weight correction for restricted cos(θ_z) range

When czMin/czMax restrict the angular acceptance to less than the full sphere, the generated events no longer represent the full phase space. If downstream code assumes isotropic generation (e.g., for Rmue normalization, which the comment on line 131 says is "per muon"), the effective solid-angle fraction should either be:

documented clearly so users know they must account for it, or
applied as a weight/correction factor.

This isn't necessarily a code bug, but it's a physics concern. Consider adding a comment or log message stating the solid-angle fraction being generated:

    if(conf().czMin() > -1. || conf().czMax() < 1.) {
      double fraction = (conf().czMax() - conf().czMin()) / 2.0;
      mf::LogInfo("CeEndpoint") << "Restricting cos(theta_z) to ["
        << conf().czMin() << ", " << conf().czMax()
        << "], solid angle fraction = " << fraction << "\n";
    }

3. 🟡 Verbosity log doesn't report the new cz parameters

The existing verbose log block (lines 107–113) reports stoppingTargetMaterial, endpoint energy, and muon lifetime, but does not log the new czMin/czMax values. This makes debugging harder when a non-default range is used.

    if(verbosity_ > 0) {
      mf::LogInfo log("CeEndpoint");
      log<<"stoppingTargetMaterial = "<<conf().stoppingTargetMaterial()
         <<", endpoint energy = "<<endPointEnergy_
         <<", muon lifetime = "<<muonLifeTime_
+        <<", czMin = "<<conf().czMin()
+        <<", czMax = "<<conf().czMax()
         <<std::endl;
    }

4. 🟢 Minor: fhicl parameter naming convention

The existing parameters use camelCase (inputSimParticles, stoppingTargetMaterial), which is consistent with the new czMin/czMax. This looks fine — just noting it passes convention review.

Summary

#	Severity	Issue
1	🔴 High	No validation on `czMin`/`czMax` — invalid ranges silently produce wrong physics
2	🟡 Medium	No documentation or warning about solid-angle normalization impact
3	🟡 Medium	New parameters not included in verbose logging
4	🟢 Info	Naming convention is consistent ✅

The code change itself is mechanically correct — the RandomUnitSphere constructor does accept the czmin/czmax arguments in exactly this form. The main concern is the lack of input guardrails and logging for the new parameters.

michaelmackenzie · 2026-04-06T15:36:32Z

I added an error throw for bad cz range values and added the values to the logged output

oksuzian · 2026-04-06T15:43:08Z

@FNALbuild build test

oksuzian · 2026-04-06T15:44:01Z

@FNALbuild run build test

FNALbuild · 2026-04-06T15:44:27Z

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

brownd1978

Would it make sense to record the acceptance of the cz cut ( 0.5*(czmax-czmin)) as a FilterFraction in the enjob? That would allow computing the net POT equivalent without having to harvest numbers from a log file.

michaelmackenzie · 2026-04-06T15:49:21Z

@brownd1978 I was thinking about this as well, either as an EventWeight (but being per-event is wasteful) or a FilterFraction (but it's not really filtering gen events). I could make a new data product SubRunWeight to cover this? Or just put EventWeight into the sub run. I don't think the FilterFraction payload/accumulation will make as much sense here, but I haven't thought about it too deeply

FNALbuild · 2026-04-06T15:49:46Z

📝 The HEAD of main has changed to 50ac9dc. Tests are now out of date.

FNALbuild · 2026-04-06T15:54:05Z

☔ The build is failing at 6c62b85.

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

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged `6c62b85` at `a31b07e`
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	➡️	2 errors 6 warnings
whitespace check	✅	no whitespace errors found

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

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 · 2026-04-06T16:06:20Z

@FNALbuild run build test

FNALbuild · 2026-04-06T16:06:33Z

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

FNALbuild · 2026-04-06T16:26:18Z

☀️ The build tests passed at 713ba67.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged `713ba67` at `50ac9dc`
build (prof)	✅	Log file. Build time: 08 min 24 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 5 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 713ba67 after being merged into the base branch at 50ac9dc.

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.

brownd1978 · 2026-04-06T18:24:13Z

In a sense it is a filter, just applied during the generation. So I think FilterFraction is adequate.

…

On Mon, Apr 6, 2026 at 8:49 AM Michael MacKenzie ***@***.***> wrote: *michaelmackenzie* left a comment (Mu2e/Offline#1782) <#1782 (comment)> @brownd1978 <https://github.com/brownd1978> I was thinking about this as well, either as an EventWeight (but being per-event is wasteful) or a FilterFraction (but it's not really filtering gen events). I could make a new data product SubRunWeight to cover this? Or just put EventWeight into the sub run. I don't think the FilterFraction payload/accumulation will make as much sense here, but I haven't thought about it too deeply — Reply to this email directly, view it on GitHub <#1782 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABAH573SNW7RAMBHZUKMBLD4UPGZRAVCNFSM6AAAAACXOH2RESVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DCOJTGI4DMMRSGQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- David Brown ***@***.*** Office Phone (510) 486-7261 Lawrence Berkeley National Lab M/S 50R5008 (50-6026C) Berkeley, CA 94720

michaelmackenzie · 2026-04-06T18:32:16Z

@brownd1978 What would you suggest for nseen_ and npassed_ for the object? As these are integers, setting the weight becomes a bit tricky in that we'd need to represent the double correction value as a fraction. It also has payloads bool chained_, uint64_t nseen_ , and uint64_t npassed_ instead of essentially the double fraction_ we'd ideally store here.

Perhaps more appropriate would be SpectrumConfig struct that could have emin_, emax_, tmin_, tmax_, czmin_, czmax_, and fraction_sampled_ for commonly configured generation values. This would also help flat electron/photon datasets that need (emax - emin) for proper normalization. Of course it wouldn't cover all cases, but this could consolidate some of these issues into a single solution struct.

brownd1978 · 2026-04-06T18:43:36Z

npassed_ = gencount (number of events generated) nseen_ = gencount/fraction. This woud be rounded, as you say, which isn't perfect but also isn't wrong. chained is an internal variable. Add and chain would work as expected. Essentially think of this as a pure phasespace generator followed by a filter. I don't see SpectrumConfig in any Mu2e repo, is that your private struct?

…

On Mon, Apr 6, 2026 at 11:32 AM Michael MacKenzie ***@***.***> wrote: *michaelmackenzie* left a comment (Mu2e/Offline#1782) <#1782 (comment)> @brownd1978 <https://github.com/brownd1978> What would you suggest for nseen_ and npassed_ for the object? As these are integers, setting the weight becomes a bit tricky in that we'd need to represent the double correction value as a fraction. It also has payloads bool chained_, uint64_t nseen_ , and uint64_t npassed_ instead of essentially the double fraction_ we'd ideally store here. Perhaps more appropriate would be SpectrumConfig struct that could have emin_, emax_, tmin_, tmax_, czmin_, czmax_, and fraction_sampled_ for commonly configured generation values. This would also help flat electron/photon datasets that need (emax - emin) for proper normalization. Of course it wouldn't cover all cases, but this could consolidate some of these issues into a single solution struct. — Reply to this email directly, view it on GitHub <#1782 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABAH57YCRHL5FLHAXMCGOZ34UPZ4PAVCNFSM6AAAAACXOH2RESVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DCOJUGIYDQNJXGU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- David Brown ***@***.*** Office Phone (510) 486-7261 Lawrence Berkeley National Lab M/S 50R5008 (50-6026C) Berkeley, CA 94720

michaelmackenzie · 2026-04-06T18:50:47Z

There isn't a SpectrumConfig struct, I meant this as a proposal. I'm often annoyed that I have to check fcl to know emin/emax or czmin/czmax, where something like this would automate this. But I don't feel strongly, just adding this as an idea to target two issues at once

brownd1978 · 2026-04-06T19:49:17Z

I see. Your idea does make sense, but interpreting this payload as a fraction requires context (ie is the E spectrum flat?). Where would that go? You should decide which solution. David Brown ***@***.*** Office Phone (510) 486-7261 Lawrence Berkeley National Lab M/S 50R5008 (50-6026C) Berkeley, CA 94720

…

On Mon, Apr 6, 2026 at 11:51 Michael MacKenzie ***@***.***> wrote: *michaelmackenzie* left a comment (Mu2e/Offline#1782) <#1782 (comment)> There isn't a SpectrumConfig struct, I meant this as a proposal. I'm often annoyed that I have to check fcl to know emin/emax or czmin/czmax, where something like this would automate this. But I don't feel strongly, just adding this as an idea to target two issues at once — Reply to this email directly, view it on GitHub <#1782 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABAH5764KMK7J74GEDMNTQT4UP4B3AVCNFSM6AAAAACXOH2RESVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DCOJUGMYTOMRZGU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Allow cz restriction with CE generation

c54976b

FNALbuild added build pending build running and removed build pending labels Apr 6, 2026

FNALbuild added build finished and removed build running labels Apr 6, 2026

Add checks for CeEndpoint cz restriction

6c62b85

FNALbuild added build pending and removed build finished labels Apr 6, 2026

FNALbuild added build running and removed build pending labels Apr 6, 2026

brownd1978 reviewed Apr 6, 2026

View reviewed changes

FNALbuild added build pending and removed build running labels Apr 6, 2026

Fix init order

713ba67

FNALbuild removed the build pending label Apr 6, 2026

FNALbuild added build pending and removed build pending labels Apr 6, 2026

FNALbuild added the build running label Apr 6, 2026

FNALbuild added build finished and removed build running labels Apr 6, 2026

oksuzian merged commit c6b091b into Mu2e:main Apr 6, 2026
14 checks passed

michaelmackenzie deleted the CzRestriction branch April 6, 2026 17:49

Conversation

michaelmackenzie commented Apr 6, 2026

Uh oh!

FNALbuild commented Apr 6, 2026

Uh oh!

FNALbuild commented Apr 6, 2026

Uh oh!

oksuzian commented Apr 6, 2026

1. 🔴 No input validation on czMin / czMax

2. 🟡 Missing normalization / weight correction for restricted cos(θ_z) range

3. 🟡 Verbosity log doesn't report the new cz parameters

4. 🟢 Minor: fhicl parameter naming convention

Summary

Uh oh!

michaelmackenzie commented Apr 6, 2026

Uh oh!

oksuzian commented Apr 6, 2026

Uh oh!

oksuzian commented Apr 6, 2026

Uh oh!

FNALbuild commented Apr 6, 2026

Uh oh!

brownd1978 left a comment

Choose a reason for hiding this comment

Uh oh!

michaelmackenzie commented Apr 6, 2026

Uh oh!

FNALbuild commented Apr 6, 2026

Uh oh!

FNALbuild commented Apr 6, 2026

Uh oh!

michaelmackenzie commented Apr 6, 2026

Uh oh!

FNALbuild commented Apr 6, 2026

Uh oh!

FNALbuild commented Apr 6, 2026

Uh oh!

Uh oh!

brownd1978 commented Apr 6, 2026 via email

Uh oh!

michaelmackenzie commented Apr 6, 2026

Uh oh!

brownd1978 commented Apr 6, 2026 via email

Uh oh!

michaelmackenzie commented Apr 6, 2026

Uh oh!

brownd1978 commented Apr 6, 2026 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

1. 🔴 No input validation on `czMin` / `czMax`