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] - chore(*): register global fact instances #11749

Closed
wants to merge 1 commit into from

Conversation

sgouezel
Copy link
Collaborator

We register globally some fact instances which are necessary for integration or euclidean spaces. And also the fact that 2 and 3 are prime. See https://leanprover.zulipchat.com/#narrow/stream/113489-new-members/topic/euclidean_space.20error/near/269992165


Open in Gitpod

@sgouezel sgouezel added the awaiting-review The author would like community review of the PR label Jan 31, 2022
@@ -71,8 +71,8 @@ def pi_Lp {ι : Type*} (p : ℝ) (α : ι → Type*) : Type* := Π (i : ι), α
instance {ι : Type*} (p : ℝ) (α : ι → Type*) [∀ i, inhabited (α i)] : inhabited (pi_Lp p α) :=
⟨λ i, default⟩

lemma fact_one_le_one_real : fact ((1:ℝ) ≤ 1) := ⟨rfl.le⟩
lemma fact_one_le_two_real : fact ((1:ℝ) ≤ 2) := ⟨one_le_two⟩
instance fact_one_le_one_real : fact ((1:ℝ) ≤ 1) := ⟨rfl.le⟩
Copy link
Member

Choose a reason for hiding this comment

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

Is there any benefit to having this specialized instance, instead of a generic le_rfl instance?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can imagine some situations where the system would want to check fact (p ≤ q) and then it could try to see if by chance p = q and run into a heavy refl computation, while this wouldn't happen if it needed to check whether p is 1. But all this is completely theoretical, I don't have a concrete example in mind. I just went for the smallest possible change.

@alexjbest
Copy link
Member

Is there a way to benchmark this change? I know performance was one of the reasons we avoided global fact instances in the past so it would be good to check that this doesn't have adverse effects

@alexjbest
Copy link
Member

Have you considered making these instances localized in some locale rather than global. I agree one of the big issues with the way we use fact is discoverability but perhaps putting them in a well-named locale would be newcomer friendly?

@sgouezel
Copy link
Collaborator Author

sgouezel commented Jan 31, 2022

I expect that these changes will have absolutely no performance impact whatsoever, because this will just be 7 global fact instances. fact instances are bad because there is no head symbol to discriminate against, so Lean will try all of them seriously. But we don't use fact that much, and all these instances can't lead to further instance searches.

Of course, I don't have concrete figures to back my claims :-)

@sgouezel
Copy link
Collaborator Author

sgouezel commented Feb 1, 2022

Jannis Limperg benchmarked the change, see https://leanprover.zulipchat.com/#narrow/stream/113489-new-members/topic/euclidean_space.20error/near/270164697. It makes no performance difference.

@jcommelin
Copy link
Member

Thanks 🎉

bors merge

@github-actions github-actions bot 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 awaiting-review The author would like community review of the PR labels Feb 1, 2022
bors bot pushed a commit that referenced this pull request Feb 1, 2022
We register globally some fact instances which are necessary for integration or euclidean spaces. And also the fact that 2 and 3 are prime. See https://leanprover.zulipchat.com/#narrow/stream/113489-new-members/topic/euclidean_space.20error/near/269992165
@bors
Copy link

bors bot commented Feb 1, 2022

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title chore(*): register global fact instances [Merged by Bors] - chore(*): register global fact instances Feb 1, 2022
@bors bors bot closed this Feb 1, 2022
@bors bors bot deleted the global_fact branch February 1, 2022 13:54
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+`.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants