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] - feat: simps uses fields of parent structures #2042

Closed
wants to merge 28 commits into from

Conversation

fpvandoorn
Copy link
Member

@fpvandoorn fpvandoorn commented Feb 3, 2023

  • initialize_simps_projections now by default generates all projections of all parent structures, and doesn't generate the projections to those parent structures.
  • You can also rename a nested projection directly, without having to specify intermediate parent structures
  • Added the option to turn the default behavior off (done in e.g. TwoPointed)

Internal changes:

  • Move most declarations to the Simps namespace, and shorten their names
  • Restructure ParsedProjectionData to avoid the bug reported here (and to another bug where it seemed that the wrong data was inserted in ParsedProjectionData, but it was hard to minimize because of all the crashes). If we manage to fix the bug in that Zulip thread, I'll see if I can track down the other bug in commit 9745428

This likely conflicts with future PRs, so please delegate instead of merge

Open in Gitpod

@fpvandoorn fpvandoorn added awaiting-review t-meta Tactics, attributes or user commands awaiting-CI labels Feb 3, 2023
Mathlib/Lean/Expr/Basic.lean Outdated Show resolved Hide resolved
Mathlib/Tactic/Simps/Basic.lean Outdated Show resolved Hide resolved
@fpvandoorn fpvandoorn added awaiting-author A reviewer has asked the author a question or requested changes and removed awaiting-review labels Feb 4, 2023
@fpvandoorn
Copy link
Member Author

fpvandoorn commented Feb 6, 2023

I separated out roughly the definitions you wrote. Thanks for that, they were much nicer than the versions I had myself earlier.
I added some additional changes to this PR:

  • I had to change mkParsedProjectionData, to put parts of it in the shape @[simps] expects (and documented these requirements)
  • I changed the rules for initialize_simps_projections so that the default behavior is better (that was already part of the previous review), but for the sake of mathlib3-backwards compatibility I added the option to turn the default behavior off and revert to the previous behavior (done in e.g. TwoPointed)
  • I restructured ParsedProjectionData, otherwise I would often run into the bug reported here (and to another bug where it seemed that the wrong data was inserted in ParsedProjectionData, but it was hard to minimize because of all the crashes). If we manage to fix the bug in that Zulip thread, I'll see if I can track down the other bug in commit 9745428

@semorrison semorrison added the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Feb 6, 2023
@semorrison semorrison removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Feb 8, 2023
@fpvandoorn fpvandoorn added awaiting-review and removed awaiting-author A reviewer has asked the author a question or requested changes labels Feb 14, 2023
@semorrison semorrison added the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Feb 28, 2023
@semorrison semorrison removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Feb 28, 2023
@kmill
Copy link
Contributor

kmill commented Feb 28, 2023

It seems like an improvement to me, and given there haven't been any more comments in the last few weeks:

bors d+

@bors
Copy link

bors bot commented Feb 28, 2023

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

@semorrison semorrison added the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Feb 28, 2023
@semorrison semorrison removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Feb 28, 2023
@fpvandoorn
Copy link
Member Author

bors merge

@github-actions github-actions bot added the ready-to-merge This PR has been sent to bors. label Feb 28, 2023
bors bot pushed a commit that referenced this pull request Feb 28, 2023
* `initialize_simps_projections` now by default generates all projections of all parent structures, and doesn't generate the projections to those parent structures.
* You can also rename a nested projection directly, without having to specify intermediate parent structures
* Added the option to turn the default behavior off (done in e.g. `TwoPointed`)

Internal changes:

* Move most declarations to the Simps namespace, and shorten their names
* Restructure `ParsedProjectionData` to avoid the bug [reported here](https://leanprover.zulipchat.com/#narrow/stream/270676-lean4/topic/allocation.20crash.3F) (and to another bug where it seemed that the wrong data was inserted in `ParsedProjectionData`, but it was hard to minimize because of all the crashes). If we manage to fix the bug in that Zulip thread, I'll see if I can track down the other bug in commit 9745428



Co-authored-by: Johan Commelin <johan@commelin.net>
@bors
Copy link

bors bot commented Feb 28, 2023

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat: simps uses fields of parent structures [Merged by Bors] - feat: simps uses fields of parent structures Feb 28, 2023
@bors bors bot closed this Feb 28, 2023
@bors bors bot deleted the simpsDefaultPlus branch February 28, 2023 22:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
delegated 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.

None yet

5 participants