Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Merged by Bors] - fix: more stable choice of representative for atoms in ring and abel #19119

Closed
wants to merge 15 commits into from

Conversation

hrmacbeth
Copy link
Member

Algebraic normalization tactics (ring, abel, etc.) typically require a concept of "atom", with expressions which are t-defeq (for some transparency t) being identified. However, the particular representative of this equivalence class which turns up in the final normalized expression is basically random. (It often ends up being the "first", in some tree sense, occurrence of the equivalence class in the expression being normalized.)

This ends up being particularly unpredictable when multiple expressions are being normalized simultaneously, e.g. with ring_nf or abel_nf: it can occur that in different expressions, a different representative of the equivalence class is chosen. For example, on current Mathlib,

example (x : ℤ) (R : ℤ → ℤ → Prop) : True := by
  let a := x
  have h : R (a + x) (x + a) := sorry
  ring_nf at h

the statement of h after the ring-normalization step is h : R (a * 2) (x * 2). Here a and x are reducibly defeq. When normalizing a + x, the representative a was chosen for the equivalence class; when normalizing x + a, the representative x was chosen.

This PR implements a fix. The AtomM monad (which is used for atom-tracking in ring, abel, etc.) has its addAtom function modified to report, not just the index of an atom, but also the stored form of the atom. Then the code surrounding addAtom calls in the ring, abel and module tactics is modified to use the stored form of the atom, rather than the form actually encountered at that point in the expression.


Open in Gitpod

@hrmacbeth hrmacbeth added the t-meta Tactics, attributes or user commands label Nov 16, 2024
Copy link

github-actions bot commented Nov 16, 2024

PR summary 86eb01d068

Import changes exceeding 2%

% File
+5.26% Mathlib.Util.AtomM

Import changes for modified files

Dependency changes

File Base Count Head Count Change
Mathlib.Util.AtomM 19 20 +1 (+5.26%)
Import changes for all files
Files Import difference
Mathlib.Util.AtomM 1

Declarations diff

+ AtomM.addAtomQ
++ myId

You can run this locally as follows
## summary with just the declaration names:
./scripts/declarations_diff.sh <optional_commit>

## more verbose report:
./scripts/declarations_diff.sh long <optional_commit>

The doc-module for script/declarations_diff.sh contains some details about this script.


No changes to technical debt.

You can run this locally as

./scripts/technical-debt-metrics.sh pr_summary
  • The relative value is the weighted sum of the differences with weight given by the inverse of the current value of the statistic.
  • The absolute value is the relative value divided by the total sum of the inverses of the current values (i.e. the weighted average of the differences).

@hrmacbeth hrmacbeth changed the title fix: more predictable choice of representative for atoms in ring and abel fix: more stable choice of representative for atoms in ring and abel Nov 16, 2024
MathlibTest/abel.lean Outdated Show resolved Hide resolved
@github-actions github-actions bot added the large-import Automatically added label for PRs with a significant increase in transitive imports label Nov 18, 2024
@Vierkantor
Copy link
Contributor

Copy link
Contributor

@Vierkantor Vierkantor left a comment

Choose a reason for hiding this comment

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

One thing I was worried about but seems to be okay is compatibility with CanonM, but we could easily add a CanonM.canon call around addAtom, so that should be all good.

@eric-wieser, any further review comments?

Mathlib/Util/AtomM.lean Outdated Show resolved Hide resolved
Mathlib/Util/AtomM.lean Outdated Show resolved Hide resolved
@hrmacbeth
Copy link
Member Author

Thanks for these improvements!

MathlibTest/ring.lean Outdated Show resolved Hide resolved
MathlibTest/abel.lean Outdated Show resolved Hide resolved
@eric-wieser
Copy link
Member

bors d+

Thanks!

@mathlib-bors
Copy link
Contributor

mathlib-bors bot commented Nov 21, 2024

✌️ hrmacbeth can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@hrmacbeth
Copy link
Member Author

bors r+

@github-actions github-actions bot added the ready-to-merge This PR has been sent to bors. label Nov 21, 2024
mathlib-bors bot pushed a commit that referenced this pull request Nov 21, 2024
…el` (#19119)

Algebraic normalization tactics (`ring`, `abel`, etc.) typically require a concept of "atom", with expressions which are t-defeq (for some transparency t) being identified.  However, the particular representative of this equivalence class which turns up in the final normalized expression is basically random.  (It often ends up being the "first", in some tree sense, occurrence of the equivalence class in the expression being normalized.)

This ends up being particularly unpredictable when multiple expressions are being normalized simultaneously, e.g. with `ring_nf` or `abel_nf`: it can occur that in different expressions, a different representative of the equivalence class is chosen.  For example, on current Mathlib,
```lean
example (x : ℤ) (R : ℤ → ℤ → Prop) : True := by
  let a := x
  have h : R (a + x) (x + a) := sorry
  ring_nf at h
```
the statement of `h` after the ring-normalization step is `h : R (a * 2) (x * 2)`.  Here `a` and `x` are reducibly defeq.  When normalizing `a + x`, the representative `a` was chosen for the equivalence class; when normalizing `x + a`, the representative `x` was chosen.

This PR implements a fix.  The `AtomM` monad (which is used for atom-tracking in `ring`, `abel`, etc.) has its `addAtom` function modified to report, not just the index of an atom, but also the stored form of the atom.  Then the code surrounding `addAtom` calls in the `ring`, `abel` and `module` tactics is modified to use the stored form of the atom, rather than the form actually encountered at that point in the expression.



Co-authored-by: Eric Wieser <[email protected]>
@mathlib-bors
Copy link
Contributor

mathlib-bors bot commented Nov 21, 2024

Pull request successfully merged into master.

Build succeeded:

@mathlib-bors mathlib-bors bot changed the title fix: more stable choice of representative for atoms in ring and abel [Merged by Bors] - fix: more stable choice of representative for atoms in ring and abel Nov 21, 2024
@mathlib-bors mathlib-bors bot closed this Nov 21, 2024
@mathlib-bors mathlib-bors bot deleted the HM-ring-bug-2 branch November 21, 2024 04:33
TobiasLeichtfried pushed a commit that referenced this pull request Nov 21, 2024
…el` (#19119)

Algebraic normalization tactics (`ring`, `abel`, etc.) typically require a concept of "atom", with expressions which are t-defeq (for some transparency t) being identified.  However, the particular representative of this equivalence class which turns up in the final normalized expression is basically random.  (It often ends up being the "first", in some tree sense, occurrence of the equivalence class in the expression being normalized.)

This ends up being particularly unpredictable when multiple expressions are being normalized simultaneously, e.g. with `ring_nf` or `abel_nf`: it can occur that in different expressions, a different representative of the equivalence class is chosen.  For example, on current Mathlib,
```lean
example (x : ℤ) (R : ℤ → ℤ → Prop) : True := by
  let a := x
  have h : R (a + x) (x + a) := sorry
  ring_nf at h
```
the statement of `h` after the ring-normalization step is `h : R (a * 2) (x * 2)`.  Here `a` and `x` are reducibly defeq.  When normalizing `a + x`, the representative `a` was chosen for the equivalence class; when normalizing `x + a`, the representative `x` was chosen.

This PR implements a fix.  The `AtomM` monad (which is used for atom-tracking in `ring`, `abel`, etc.) has its `addAtom` function modified to report, not just the index of an atom, but also the stored form of the atom.  Then the code surrounding `addAtom` calls in the `ring`, `abel` and `module` tactics is modified to use the stored form of the atom, rather than the form actually encountered at that point in the expression.



Co-authored-by: Eric Wieser <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
delegated large-import Automatically added label for PRs with a significant increase in transitive imports ready-to-merge This PR has been sent to bors. t-meta Tactics, attributes or user commands
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants