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

fix(tactic/ring): more precise pattern match for div #1557

Merged
merged 13 commits into from Feb 28, 2020
Merged

fix(tactic/ring): more precise pattern match for div #1557

merged 13 commits into from Feb 28, 2020

Conversation

digama0
Copy link
Member

@digama0 digama0 commented Oct 16, 2019

No description provided.

@PatrickMassot
Copy link
Member

Could you please add a test showing the improvement?

@@ -386,7 +386,7 @@ meta def eval : expr → ring_m (horner_expr × expr)
(e', p) ← lift $ norm_num.derive e,
lift $ e'.to_rat,
return (const e', p)) <|> eval_atom e
| `(%%e₁ / %%e₂) := do
| `(@has_div.div _ division_ring_has_div %%e₁ %%e₂) := do
Copy link
Member

Choose a reason for hiding this comment

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

Do you think the syntactic check here is too strong? What about matching the instance as %%inst and checking that it unifies with an application of division_ring_has_div?

Copy link
Member Author

Choose a reason for hiding this comment

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

@robertylewis I suppose that's a possibility, but I don't actually think it comes up; there aren't any other competing instances for has_div that I'm aware of, everything filters through this instance. (There are many ways to prove the resulting division_ring goal, but AFAIK the only has_div instances are the ones for nat/int and the division ring div.)

Copy link
Member

Choose a reason for hiding this comment

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

I dunno, that doesn't sound very robust. Is the current Travis failure related? (I'm not gonna compile it on my laptop right now.)

Copy link
Member

Choose a reason for hiding this comment

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

There's another instance called division_ring_has_div'. This is causing the current travis failure.

@robertylewis robertylewis self-assigned this Oct 16, 2019
@robertylewis robertylewis added the awaiting-author A reviewer has asked the author a question or requested changes label Oct 22, 2019
@robertylewis
Copy link
Member

@digama0 Are you planning to come back to this?

@digama0 digama0 removed the awaiting-author A reviewer has asked the author a question or requested changes label Oct 28, 2019
@robertylewis
Copy link
Member

It looks like the breakage is ring failing to close a goal it used to close, combined with a proof missing focusing { }.

@robertylewis
Copy link
Member

@digama0 ping again?

@robertylewis
Copy link
Member

@digama0 ping again.

@fpvandoorn fpvandoorn added the WIP Work in progress label Nov 19, 2019
@robertylewis
Copy link
Member

@digama0 IIRC you were having trouble replicating this build failure locally. Is that still the case? I can locally reproduce the CI failure.

@robertylewis robertylewis added ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.) and removed WIP Work in progress labels Feb 28, 2020
@mergify mergify bot merged commit 4149099 into master Feb 28, 2020
@mergify mergify bot deleted the ring-div branch February 28, 2020 13:50
anrddh pushed a commit to anrddh/mathlib that referenced this pull request May 15, 2020
…unity#1557)

* fix(tactic/ring): more precise pattern match for div

* add test

* fix instance check for div

* chore(algebra/quadratic_discriminant): add braces in have steps

* use norm_num instead of ring to evaluate exponents

* fix norm_num uses

* fix norm_num pow bug

* bugfix

* fix proof

Co-authored-by: Rob Lewis <Rob.y.lewis@gmail.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
anrddh pushed a commit to anrddh/mathlib that referenced this pull request May 16, 2020
…unity#1557)

* fix(tactic/ring): more precise pattern match for div

* add test

* fix instance check for div

* chore(algebra/quadratic_discriminant): add braces in have steps

* use norm_num instead of ring to evaluate exponents

* fix norm_num uses

* fix norm_num pow bug

* bugfix

* fix proof

Co-authored-by: Rob Lewis <Rob.y.lewis@gmail.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
@bryangingechen bryangingechen added the t-meta Tactics, attributes or user commands label Feb 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.) t-meta Tactics, attributes or user commands
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants