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(number_theory): pythagorean triples #3200

Closed
wants to merge 17 commits into from

Conversation

paulvanwamelen
Copy link
Collaborator

@paulvanwamelen paulvanwamelen commented Jun 27, 2020

The classification of pythagorean triples (one of the "100 theorems")


src/data/int/gcd.lean Outdated Show resolved Hide resolved
src/data/nat/prime.lean Outdated Show resolved Hide resolved
src/number_theory/pythagorean_triples.lean Outdated Show resolved Hide resolved
Copy link
Member

@jcommelin jcommelin left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I left some comments.
I think that his PR should include some simp-lemmas about the circle parameterisation. If you apply the equiv to a term of k, what does the result look like? (I know the answer, but simp doesn't.) Similarly, if you have a point on the circle, and you apply the equiv.symm, which element of k do you get?

src/algebra/gcd_domain.lean Outdated Show resolved Hide resolved
src/data/nat/prime.lean Outdated Show resolved Hide resolved
src/data/rat/basic.lean Outdated Show resolved Hide resolved
src/data/rat/basic.lean Outdated Show resolved Hide resolved
src/number_theory/pythagorean_triples.lean Outdated Show resolved Hide resolved
src/number_theory/pythagorean_triples.lean Outdated Show resolved Hide resolved
src/number_theory/pythagorean_triples.lean Outdated Show resolved Hide resolved
src/number_theory/pythagorean_triples.lean Outdated Show resolved Hide resolved
src/number_theory/pythagorean_triples.lean Outdated Show resolved Hide resolved
@paulvanwamelen
Copy link
Collaborator Author

Are these the simp lemmas you had in mind:

lemma circle_equiv_inv (hk : ∀ x : K, 1 + x^2 ≠ 0) (v : K) (w : K) (hp : v^2 + w^2 = 1 ∧ w ≠ -1) :
  (circle_equiv_gen hk).inv_fun ⟨⟨v, w⟩, hp⟩ = v / (w + 1) := by refl

lemma circle_equiv_fun (hk : ∀ x : K, 1 + x^2 ≠ 0) (x : K) :
  ((circle_equiv_gen hk).to_fun x).val = ⟨2 * x / (1 + x^2), (1 - x^2) / (1 + x^2)⟩ := by refl

?

src/data/nat/prime.lean Outdated Show resolved Hide resolved
src/data/rat/basic.lean Outdated Show resolved Hide resolved
@jcommelin
Copy link
Member

Are these the simp lemmas you had in mind:

lemma circle_equiv_inv (hk : ∀ x : K, 1 + x^2 ≠ 0) (v : K) (w : K) (hp : v^2 + w^2 = 1 ∧ w ≠ -1) :
  (circle_equiv_gen hk).inv_fun ⟨⟨v, w⟩, hp⟩ = v / (w + 1) := by refl

lemma circle_equiv_fun (hk : ∀ x : K, 1 + x^2 ≠ 0) (x : K) :
  ((circle_equiv_gen hk).to_fun x).val = ⟨2 * x / (1 + x^2), (1 - x^2) / (1 + x^2)⟩ := by refl

?

Almost, yes. But replace inv_fun with symm, and let ((v,w), hp) be an arbitrary term of the subtype: (x : { p : K × K // ... }).
Remove .to_fun, and replace .val by a coercion.

@bryangingechen bryangingechen added the awaiting-review The author would like community review of the PR label Jun 28, 2020
@jcommelin
Copy link
Member

I suggest adding a bit more material to the pythagorean triples file. How much extra work (lines of code) do you think the classification is?

@paulvanwamelen
Copy link
Collaborator Author

The whole file is currently 500 lines. I can add about 200 more lines of silly lemmas that are needed. Or should I just add the whole thing?

@jcommelin
Copy link
Member

Maybe just add the whole thing, if you don't need any changes to other files. Otherwise, I suggest a "preparations for" PR, that does all the changes you need to existing files, and then a "new file" PR.

@paulvanwamelen
Copy link
Collaborator Author

OK, I'll add the rest now. I don't need more changes to other files.

@semorrison semorrison added awaiting-author A reviewer has asked the author a question or requested changes and removed awaiting-review The author would like community review of the PR labels Jun 29, 2020
src/algebra/gcd_domain.lean Outdated Show resolved Hide resolved
src/algebra/gcd_domain.lean Outdated Show resolved Hide resolved
src/algebra/gcd_domain.lean Outdated Show resolved Hide resolved
src/algebra/gcd_domain.lean Outdated Show resolved Hide resolved
src/algebra/group_power.lean Show resolved Hide resolved
@paulvanwamelen paulvanwamelen added the awaiting-review The author would like community review of the PR label Jul 6, 2020
src/data/int/basic.lean Show resolved Hide resolved
src/number_theory/pythagorean_triples.lean Outdated Show resolved Hide resolved
@semorrison
Copy link
Collaborator

@paulvanwamelen, I can see you've marked this as request-review again, but there are many unresolved conversations above, so it's hard for me to review this without having to re-read everything. Could you please mark all resolved conversations as resolved, and if there are any outstanding ones (or ones where'd you like someone else to confirm they've successfully been resolved), comment on them?

@semorrison
Copy link
Collaborator

There are also some linting errors that need to be fixed.

@semorrison semorrison added awaiting-author A reviewer has asked the author a question or requested changes and removed awaiting-review The author would like community review of the PR labels Jul 8, 2020
@paulvanwamelen
Copy link
Collaborator Author

I haven't had time to properly look at this, but the linter complains that the definition of is_classified and is_primitive_classified doesn't depend on the fact that x y and z form a pythagorean triple. And, of course, the linter is correct: is_classified is just a property of x and y. On the other hand it is nice to be able to say h.is_classified. So what should be done? Include something like x * x + y * y = z * z in the condition for is_classified? Or just tell the linter to ignore the problem?

@paulvanwamelen
Copy link
Collaborator Author

I closed most comments. There are one or 2 still open above, some for me and one for Scott.

@jcommelin
Copy link
Member

@paulvanwamelen Just add @[nolint unused_arguments] before those definitions.

@paulvanwamelen
Copy link
Collaborator Author

I believe everything has been addressed now. Ready for another review.

@paulvanwamelen paulvanwamelen added awaiting-review The author would like community review of the PR and removed awaiting-author A reviewer has asked the author a question or requested changes labels Jul 9, 2020
Copy link
Member

@jcommelin jcommelin left a comment

Choose a reason for hiding this comment

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

This is looking good now! Almost ready for merging.

src/number_theory/pythagorean_triples.lean Outdated Show resolved Hide resolved
@robertylewis robertylewis added awaiting-author A reviewer has asked the author a question or requested changes and removed awaiting-review The author would like community review of the PR labels Jul 9, 2020
…umentation of the method used and the structure of the proof.
@paulvanwamelen paulvanwamelen added awaiting-review The author would like community review of the PR and removed awaiting-author A reviewer has asked the author a question or requested changes labels Jul 9, 2020
Copy link
Member

@jcommelin jcommelin 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 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 Jul 10, 2020
bors bot pushed a commit that referenced this pull request Jul 10, 2020
The classification of pythagorean triples (one of the "100 theorems")



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

bors bot commented Jul 10, 2020

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat(number_theory): pythagorean triples [Merged by Bors] - feat(number_theory): pythagorean triples Jul 10, 2020
@bors bors bot closed this Jul 10, 2020
@bors bors bot deleted the pythagorean_triples_v2 branch July 10, 2020 13:32
cipher1024 pushed a commit to cipher1024/mathlib that referenced this pull request Mar 15, 2022
The classification of pythagorean triples (one of the "100 theorems")



Co-authored-by: paulvanwamelen <30371019+paulvanwamelen@users.noreply.github.com>
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