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(data/matrix/hadamard): add the Hadamard product #8956

Closed
wants to merge 37 commits into from

Conversation

l534zhan
Copy link
Collaborator

@l534zhan l534zhan commented Sep 1, 2021


Open in Gitpod

@l534zhan l534zhan added the awaiting-review The author would like community review of the PR label Sep 1, 2021
@l534zhan
Copy link
Collaborator Author

l534zhan commented Sep 1, 2021

This is a PR as a part of a large project mentioned in https://leanprover.zulipchat.com/#narrow/stream/113489-new-members/topic/Contribute.20a.20project.

@l534zhan
Copy link
Collaborator Author

l534zhan commented Sep 1, 2021

I got the reported error:
/- The decidable_classical linter reports: -/
/- USES OF decidable SHOULD BE REPLACED WITH classical IN THE PROOF. -/
-- data/matrix/hadamard.lean
#print matrix.sum_Hadamard_eq /- The following decidable hypotheses should be replaced with
classical in the proof. argument 12: [_inst_12 : decidable_eq I], argument 13: [_inst_13 : decidable_eq J] -/

Why should I do this change?

@ericrbg
Copy link
Collaborator

ericrbg commented Sep 1, 2021

get rid of those parameters and start the proof with classical or something like that

@l534zhan
Copy link
Collaborator Author

l534zhan commented Sep 1, 2021

get rid of those parameters and start the proof with classical or something like that

I just don't understand, why I have to do this?

Do you have any idea? @ericrbg @eric-wieser

@ericrbg
Copy link
Collaborator

ericrbg commented Sep 1, 2021

it's unneeded, basically. the decidable_eq in that lemma is just used in an intermediate step (have h:= trace_identity A B (λ i, 1 : I → α) (λ i, 1 : J → α)) and not in the type of the lemma, so why require that parameter when it's irrelevant?

@ericrbg
Copy link
Collaborator

ericrbg commented Sep 1, 2021

i'm sort of finding it hard to say exactly what I mean, but the main essential difference is that trace_identity needs decidable_eq for diagonal, but sum_Hadamard_eq only uses diagonal in the middle and gets out of it fast anyways.

@l534zhan
Copy link
Collaborator Author

l534zhan commented Sep 1, 2021

i'm sort of finding it hard to say exactly what I mean, but the main essential difference is that trace_identity needs decidable_eq for diagonal, but sum_Hadamard_eq only uses diagonal in the middle and gets out of it fast anyways.

So that still means [decidable_eq I] [decidable_eq J] are necessary conditions for all the lemmas there. Why does linter complain? Should I only use [decidable_eq I] [decidable_eq J] as conditions when the formulation needs this and use classical for the others?
@ericrbg

@ericrbg
Copy link
Collaborator

ericrbg commented Sep 1, 2021

so mathlib as a library overall accepts Choice (⇒ LEM). therefore, in some ways, decidable_eq is kind of meaningless as we can always create some decidable equality (using choice). However, if we just blindly do this, we end up with many issues; there's a lot of decidable_eq instances floating around, and although they're all obviously propositionally equal, they're not defeq. this leads to weird rewriting errors that look something like this:

rewrite tactic failed, did not find instance of the pattern in the target expression
  α
state:
...
...
...
⊢ α = β

because in the first α has an implicit classical.decidable_eq whilst the second has an implicit my_fancy_instance.decidable_eq or whatever. these are always a pain to flag down, and so we give some leeway; whenever, in the output type, there's a decidable_eq argument hidden in somewhere, we pass decidable_eq arguments to the lemma itself so you don't have to deal with these issues. (this happens often with fintype too).

now, in your proof, you don't actually write down diagonal in the output type, so there's no such issues; you use it internally; this means that there's no issues with these "diamonds" for the rest. furthermore, even if you were trying to avoid choice, you also already use classical.choice anyways (try #print axioms ...) so you're not avoiding anything at all.

@eric-wieser eric-wieser added the awaiting-author A reviewer has asked the author a question or requested changes label Sep 2, 2021
@eric-wieser eric-wieser removed the awaiting-review The author would like community review of the PR label Sep 2, 2021
l534zhan and others added 3 commits September 2, 2021 16:42
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
@l534zhan
Copy link
Collaborator Author

l534zhan commented Sep 2, 2021

Any further improvement or suggestions? @eric-wieser
Thanks for all your suggestions, which enable me to write better codework in the future!

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.

LGTM, but I think @Vierkantor should take a quick look at the trace statements to check we're not missing a simpler result.

src/data/matrix/hadamard.lean Outdated Show resolved Hide resolved
Copy link
Collaborator

@Vierkantor Vierkantor left a comment

Choose a reason for hiding this comment

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

I believe dot_product_vec_mul_hadamard is AFAIK the standard formulation of that statement, and the complexity of the proof is not so bad. So I don't really know a simpler result.

Copy link
Collaborator

@Vierkantor Vierkantor left a comment

Choose a reason for hiding this comment

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

Thanks 🎉

bors r+

@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 Sep 6, 2021
bors bot pushed a commit that referenced this pull request Sep 6, 2021


Co-authored-by: l534zhan <84618936+l534zhan@users.noreply.github.com>
@bors
Copy link

bors bot commented Sep 6, 2021

Build failed (retrying...):

bors bot pushed a commit that referenced this pull request Sep 6, 2021


Co-authored-by: l534zhan <84618936+l534zhan@users.noreply.github.com>
@bors
Copy link

bors bot commented Sep 6, 2021

Build failed (retrying...):

@l534zhan
Copy link
Collaborator Author

l534zhan commented Sep 6, 2021

What's the problem here? @Vierkantor

bors bot pushed a commit that referenced this pull request Sep 6, 2021


Co-authored-by: l534zhan <84618936+l534zhan@users.noreply.github.com>
@bors
Copy link

bors bot commented Sep 7, 2021

Build failed (retrying...):

@bryangingechen
Copy link
Collaborator

What's the problem here? @Vierkantor

There aren't any problems here as far as I can tell. We merge our PRs in batches using bors, so a build failure could be due to any of the PRs in a batch or an interaction between any subset of them. Generally, if the bot posts a message that says "retrying...", you can just keep waiting; oftentimes the issue is elsewhere and your PR will get merged in a later batch. If it posts a "Build failed" message without "retrying" though, then that means the problem is with your PR and you should click the link to see what exactly failed.

bors bot pushed a commit that referenced this pull request Sep 7, 2021


Co-authored-by: l534zhan <84618936+l534zhan@users.noreply.github.com>
@bors
Copy link

bors bot commented Sep 7, 2021

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat(data/matrix/hadamard): add the Hadamard product [Merged by Bors] - feat(data/matrix/hadamard): add the Hadamard product Sep 7, 2021
@bors bors bot closed this Sep 7, 2021
@bors bors bot deleted the Had_prod_lz branch September 7, 2021 08:00
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

7 participants