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: Handle powers and multiplicative inverses in cancel_denoms #7819

Closed
wants to merge 5 commits into from

Conversation

sebzim4500
Copy link
Collaborator

@sebzim4500 sebzim4500 commented Oct 21, 2023

fixes #7732


Open in Gitpod

Also solves the specific example in #7808, but I would imagine that there are still cases where cancel_denoms is not idempotent.

@sebzim4500 sebzim4500 added awaiting-review t-meta Tactics, attributes or user commands labels Oct 21, 2023
@grunweg
Copy link
Collaborator

grunweg commented Oct 21, 2023

Drive-by comment: I think the proper way to format things (so they can be picked up by github) is to make "feat: Handle powers and multiplicative inverses in cancel_denoms" the PR title. The "fixes ..." part should go above the line, so it will become part of the eventual commit message.

Thanks for doing this, I'm always excited to see tactic improvements!

@sebzim4500 sebzim4500 changed the title Improvements to cancel_denoms feat: Handle powers and multiplicative inverses in cancel_denoms Oct 21, 2023
@robertylewis robertylewis self-assigned this Oct 21, 2023
@robertylewis
Copy link
Member

This is great -- thanks for the quick fix!

bors merge

@github-actions github-actions bot added ready-to-merge This PR has been sent to bors. and removed awaiting-review labels Oct 21, 2023
bors bot pushed a commit that referenced this pull request Oct 21, 2023
fixes #7732 



Co-authored-by: Rob Lewis <Rob.y.lewis@gmail.com>
@@ -177,6 +197,24 @@ partial def mkProdPrf (α : Q(Type u)) (sα : Q(Field $α)) (v : ℕ) (t : Tree
| t, ~q(-$e) => do
let v ← mkProdPrf α sα v t e
mkAppM ``CancelDenoms.neg_subst #[v]
| .node _ lhs@(.node k1 _ _) (.node k2 .nil .nil), ~q($e1 ^ $e2) => do
Copy link
Member

Choose a reason for hiding this comment

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

Probably better as

Suggested change
| .node _ lhs@(.node k1 _ _) (.node k2 .nil .nil), ~q($e1 ^ $e2) => do
| .node _ lhs@(.node k1 _ _) (.node k2 .nil .nil), ~q($e1 ^ ($e2 : ℕ)) => do

Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

It would be cool to handle zpow here too, such as 2 ^ (-1); but it's very reasonable to dexlare that out of scope.

@bors
Copy link

bors bot commented Oct 21, 2023

Pull request successfully merged into master.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot changed the title feat: Handle powers and multiplicative inverses in cancel_denoms [Merged by Bors] - feat: Handle powers and multiplicative inverses in cancel_denoms Oct 21, 2023
@bors bors bot closed this Oct 21, 2023
@bors bors bot deleted the cancel-denoms-improvements branch October 21, 2023 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

linarith Cannot Handle 2⁻¹
4 participants