Skip to content

refactor(Combinatorics/SimpleGraph): no proof obligation in rotate#36814

Open
YaelDillies wants to merge 3 commits intoleanprover-community:masterfrom
YaelDillies:walk_rotate_no_proof
Open

refactor(Combinatorics/SimpleGraph): no proof obligation in rotate#36814
YaelDillies wants to merge 3 commits intoleanprover-community:masterfrom
YaelDillies:walk_rotate_no_proof

Conversation

@YaelDillies
Copy link
Copy Markdown
Contributor

If the walk doesn't go through the new vertex, return nil instead.


Open in Gitpod

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 18, 2026

PR summary 28f63a901b

Import changes for modified files

No significant changes to the import graph

Import changes for all files
Files Import difference

Declarations diff

+ append_eq_nil
+ rotate_of_mem_support
+ rotate_of_not_mem_support

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

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

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


No changes to technical debt.

You can run this locally as

./scripts/reporting/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).

@github-actions github-actions bot added the t-combinatorics Combinatorics label Mar 18, 2026
@eric-wieser
Copy link
Copy Markdown
Member

Could you elaborate on the tradeoff here in the description? The obvious downside is that the proof can no longer be found by unification in lemmas about the definition. Are there enough places where having a total (junk) function is beneficial to warrant this loss?

@SnirBroshi
Copy link
Copy Markdown
Collaborator

Doesn't this mean that goals with rotate have to give the proof obligation for every lemma in rw, rather than it being inferred from the rotate call?

@mathlib-merge-conflicts mathlib-merge-conflicts bot added the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Mar 19, 2026
@mathlib-merge-conflicts
Copy link
Copy Markdown

This pull request has conflicts, please merge master and resolve them.

If the walk doesn't go through the new vertex, return `nil` instead.
@YaelDillies YaelDillies force-pushed the walk_rotate_no_proof branch from 71af528 to 02c9f4f Compare March 19, 2026 10:40
@github-actions github-actions bot removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Mar 19, 2026
@YaelDillies
Copy link
Copy Markdown
Contributor Author

YaelDillies commented Mar 19, 2026

I understand the tradeoff. I do not have an application for removing the proof field and opened the PR solely because we do tend to prefer junk values over propositional arguments (eg integral doesn't carry around a proof of integrability of the function) and that @b-mehta agreed it could be a good idea. On the other hand, you can see that the proof obligations aren't provided more often with this new setup: they are merely provided at a different point in the proof.

@b-mehta
Copy link
Copy Markdown
Contributor

b-mehta commented Apr 1, 2026

Yeah I agree this definition feels nicer because it doesn't require a hypothesis in the def, but Eric's point about the unification is a good one. In the diff, it's true that proof obligations aren't provided more, but that could be because the places this def is used in mathlib are simple enough that the downside doesn't really appear. Maybe the sensible thing to do is shelve this PR until there's a more concrete signal that the junk value is useful, as Eric suggests

@YaelDillies
Copy link
Copy Markdown
Contributor Author

Do we have a good way to "shelve" a PR?

@mathlib-merge-conflicts mathlib-merge-conflicts bot added the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Apr 1, 2026
@mathlib-merge-conflicts
Copy link
Copy Markdown

This pull request has conflicts, please merge master and resolve them.

@github-actions github-actions bot removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Apr 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

t-combinatorics Combinatorics

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants