Skip to content

fix: switch from ∧ to && in ZModCharges - #1704

Closed
wdconinc wants to merge 3 commits into
leanprover-community:masterfrom
wdconinc:patch-3
Closed

wdconinc wants to merge 3 commits into
leanprover-community:masterfrom
wdconinc:patch-3

Conversation

@wdconinc

Copy link
Copy Markdown
Contributor

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).

Copilot AI balanced review requested due to automatic review settings September 30, 2026 21:15
@github-actions github-actions Bot added the small label Sep 30, 2026
@github-actions

Copy link
Copy Markdown
Contributor

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.

  1. Some automated checks will be run on your PR. You can see the results of these checks at the buttom of your PR page. If any of these checks fail, you will need to fix the issues before your PR can be merged. You can learn more about these here, including how to run them locally, which is sometimes quicker than relying on the GitHub Actions. If you have never had a PR merged before, you may have to wait for a reviewer to manually start these checks (this is for security).

  2. A reviewer will look at your PR and may ask you to make changes. This may happen a couple of days after you submit your PR, so you may need to be patient. But it should not be longer than that - if it is please bring it to the attention of the community on the Zulip. The level of review will depend on where your PR is submitted. If it is submitted to ./Physlib or ./QuantumInfo, the review will be more thorough than if it is submitted to ./PhyslibAlpha. You can find out more about what the review process is looking for in our review guidelines. If a reviewer adds an awaiting-author label to your PR, address the review comments, then please remove that label by adding a comment with -awaiting-author. This helps us keep track of reviews.

  3. The reviewer will either approve your PR, or request more changes (in which case we return to step 2). Once your PR is approved, it will be merged by a maintainer, this should happen shortly after approval, though you may get more comments at this stage.

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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ZModCharges filter 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.

@jstoobysmith

Copy link
Copy Markdown
Member

Closing this, as hopefully the Lean fix will ensure this works.

@wdconinc
wdconinc deleted the patch-3 branch October 2, 2026 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants