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

chore(*): redefine {nat,int} mul based on a left-smul #6773

Closed
wants to merge 43 commits into from

Conversation

pechersky
Copy link
Collaborator


Open in Gitpod

Previously, there was a try_for to make sure the types check. But now
the type checking of the result times out, so to make the tests work,
the try_for is removed entirely. This should be investigated further.
But in any case, norm_num is able to discharge the goal, and the
kernel does not complain, so that must mean that the proof does
typecheck properly.
@pechersky
Copy link
Collaborator Author

Some issues still need to be ironed out, like why the dec_trivial proof needed to change in sampleable and why does doing the type_check in the norm_num test comlpetely time out.

@pechersky pechersky requested a review from sgouezel March 19, 2021 20:12
@github-actions github-actions bot added the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Mar 19, 2021
@pechersky pechersky added the RFC Request for comment label Mar 19, 2021
@github-actions github-actions bot removed the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Mar 21, 2021
@pechersky pechersky added the awaiting-review The author would like community review of the PR label Mar 22, 2021
@@ -33,8 +33,7 @@ example : (6:real) < 10 := by norm_num
example : (7:real)/2 > 3 := by norm_num
example : (4:real)⁻¹ < 1 := by norm_num
example : ((1:real) / 2)⁻¹ = 2 := by norm_num
example : 2 ^ 17 - 1 = 131071 :=
by {norm_num, tactic.try_for 200 (tactic.result >>= tactic.type_check)}
example : 2 ^ 17 - 1 = 131071 := by norm_num
Copy link
Member

Choose a reason for hiding this comment

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

Why did this test change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here was my commit message for that change:

remove try_for in norm_num test …
8b9b65a
Previously, there was a try_for to make sure the types check. But now
the type checking of the result times out, so to make the tests work,
the try_for is removed entirely. This should be investigated further.
But in any case, norm_num is able to discharge the goal, and the
kernel does not complain, so that must mean that the proof does
typecheck properly.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, this gives me a deep recursion error:

example : 2 ^ 17 - 1 = 131071 :=
by {norm_num, tactic.result >>= tactic.type_check}

I wonder if we're missing something subtle here. The PR introducing this test (#1387) doesn't really explain why type-checking performance is important. It works for 16 and 18, but 17 fails. @digama0 ?

Copy link
Member

Choose a reason for hiding this comment

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

The reason why typechecking performance is relevant here is because whenever norm_num is working over nat or int, everything is true by refl but this is absolutely not the proof norm_num is supposed to be producing. So if there are any bugs in the application of lemmas, norm_num will not fail, it will just take a long time to typecheck the result. If all lemmas are syntactic matches, nothing expensive should be unfolded and this is a way to ensure that this is happening.

Copy link
Member

Choose a reason for hiding this comment

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

The 2 ^ 17 - 1 = 131071 example is also not random, it was actually part of a bug report and the test was added to ensure it did not regress (and it sounds like it did regress in this version).

Copy link
Member

Choose a reason for hiding this comment

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

Here's a minimized version that eliminates norm_num:

example : 10000 * 1 = 10000 := (@mul_one ℕ nat.monoid 10000 : _)

This takes longer to typecheck depending on the size of 10000 here, which means it's not actually matching the goal like it should and is instead directly computing both sides to refl.

@pechersky
Copy link
Collaborator Author

pechersky commented Apr 7, 2021 via email

@github-actions github-actions bot added the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Apr 7, 2021
@pechersky pechersky closed this Jul 7, 2021
@pechersky pechersky deleted the pechersky/mul-on-left branch November 29, 2021 04:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review The author would like community review of the PR merge-conflict Please `git merge origin/master` then a bot will remove this label. RFC Request for comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants