Conversation
|
Thank you for this pull-request (PR). If this is your first PR, welcome to the community! Below is what will happen next. Please read carefully if you are not familiar with the process. You may open other PRs while this one is being reviewed, and can stack PRs on top of each other, so don't let these steps slow you down.
Tip: The easiest way to get have a fast review is to submit a PR that is small and self-contained, and has clear documentation explaining why things are the way they are in your chages. If you have any problems or questions, please reach out to the community on the Zulip. |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The single-line change is a semantically-equivalent boolean rewrite that fixes a reduction-time performance regression without altering the computed set or the dependent lemmas.
Review effort: Balanced
Findings: None
What changed in this PR
This PR addresses a build-time performance regression in the SU(5) charge-spectrum machinery. In ZModCharges, the Finset.filter predicate that selects complete, non-pheno-constrained, non-dangerous charge spectra is rewritten from a Prop-valued conjunction (∧) into a Bool-valued conjunction using decide and &&. Per the description, this restores short-circuiting during kernel/native_decide reduction under lean4.35.0-rc, turning previously non-terminating builds (notably n = 6) into finite ones. The change is semantically equivalent—A ∧ ¬B ∧ ¬C matches decide A && !decide B && !decide C—so the downstream decide/native_decide lemmas asserting specific set contents (ZModCharges_four_eq, ZModCharges_six_eq, etc.) continue to pin the same results.
Changes:
- Rewrites the
ZModChargesfilter predicate from∧/¬(Prop) to&&/!/decide(Bool) to recover short-circuiting during reduction. - No change to the mathematical meaning of the definition or its dependent lemmas.
| File | Description |
|---|---|
Physlib/Particles/SuperSymmetry/SU5/ChargeSpectrum/ZMod.lean |
Converts the ZModCharges filter condition to a boolean expression to fix reduction-time performance while preserving the resulting Finset. |
Note (non-blocking, outside the changed region): the sibling definition ZModZModCharges (lines 168–171) still uses the original ∧ pattern. It currently has no decide/native_decide lemmas depending on it, so the regression is latent there, but applying the same transformation would keep the two definitions consistent if such lemmas are added later.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Closing this, as hopefully the Lean fix will ensure this works. |
This PR changes the ∧ to && in ZModCharges to ensure this runs in finite time with lean4.35.0-rc1 (and -rc3).
This change is necessary (after some Claude debugging) due to changes short-circuiting behavior for ∧ but not &&. These changes were introduced in leanprover/lean4#8309, leanprover/lean4#14859, and leanprover/lean4#15128, but to be honest I am not quite clear on how or why.
This does lead to build times that are smaller (than infinite...; I never got this to build for n = 6 with lean4.34.0). This may resolve the reason why leanprover-community/downstream-reports#96 was timing out (but of course doesn't address other lean4.35.0-rc1 bumps required).