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(linear_algebra/alternating): Add dom_coprod #5269

Closed
wants to merge 1 commit into from

Conversation

eric-wieser
Copy link
Member

@eric-wieser eric-wieser commented Dec 7, 2020

This implements a variant of the multiplication defined in the second half of Proposition 22.24 of "Notes on Differential Geometry and Lie Groups" (Jean Gallier):

image


Before this gets merged I'll want to squash the commits to remove @zhangir-azerbayev from the history, as none of the code changed in this PR is theirs.

Resolved Dependencies

@eric-wieser eric-wieser added the WIP Work in progress label Dec 7, 2020
@github-actions github-actions bot added merge-conflict Please `git merge origin/master` then a bot will remove this label. blocked-by-other-PR This PR depends on another PR which is still in the queue. A bot manages this label via PR comment. and removed merge-conflict Please `git merge origin/master` then a bot will remove this label. labels Dec 8, 2020
@github-actions github-actions bot added merge-conflict Please `git merge origin/master` then a bot will remove this label. and removed merge-conflict Please `git merge origin/master` then a bot will remove this label. blocked-by-other-PR This PR depends on another PR which is still in the queue. A bot manages this label via PR comment. labels Dec 11, 2020
@@ -0,0 +1,80 @@
import group_theory.perm.sign
Copy link
Member Author

Choose a reason for hiding this comment

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

This file is junk, I'll delete it when I next push.

@github-actions github-actions bot added blocked-by-other-PR This PR depends on another PR which is still in the queue. A bot manages this label via PR comment. merge-conflict Please `git merge origin/master` then a bot will remove this label. and removed blocked-by-other-PR This PR depends on another PR which is still in the queue. A bot manages this label via PR comment. merge-conflict Please `git merge origin/master` then a bot will remove this label. labels Dec 14, 2020
@eric-wieser eric-wieser added the blocked-by-other-PR This PR depends on another PR which is still in the queue. A bot manages this label via PR comment. label Dec 17, 2020
@eric-wieser eric-wieser force-pushed the eric-wieser/alternating_map_product branch from fe37f4b to 4902e95 Compare December 17, 2020 23:11
@github-actions github-actions bot added blocked-by-other-PR This PR depends on another PR which is still in the queue. A bot manages this label via PR comment. and removed blocked-by-other-PR This PR depends on another PR which is still in the queue. A bot manages this label via PR comment. labels Dec 18, 2020
@github-actions github-actions bot added merge-conflict Please `git merge origin/master` then a bot will remove this label. and removed merge-conflict Please `git merge origin/master` then a bot will remove this label. labels Jan 3, 2021
@eric-wieser eric-wieser force-pushed the eric-wieser/alternating_map_product branch from fe04aaf to 1740bb0 Compare January 4, 2021 11:17
@github-actions github-actions bot added merge-conflict Please `git merge origin/master` then a bot will remove this label. and removed merge-conflict Please `git merge origin/master` then a bot will remove this label. blocked-by-other-PR This PR depends on another PR which is still in the queue. A bot manages this label via PR comment. labels Jan 4, 2021
@github-actions github-actions bot added the blocked-by-other-PR This PR depends on another PR which is still in the queue. A bot manages this label via PR comment. label Feb 11, 2021
bors bot pushed a commit that referenced this pull request Feb 11, 2021
@github-actions github-actions bot removed the blocked-by-other-PR This PR depends on another PR which is still in the queue. A bot manages this label via PR comment. label Feb 12, 2021
@github-actions
Copy link

🎉 Great news! Looks like all the dependencies have been resolved:

💡 To add or remove a dependency please update this issue/PR description.

Brought to you by Dependent Issues (:robot: ). Happy coding!

Comment on lines 527 to 532
with_cases {
cases hi : σ⁻¹ i;
cases hj : σ⁻¹ j;
rw perm.inv_eq_iff_eq at hi hj;
propagate_tags' { substs hi hj, }, },
case [sum.inl sum.inr : i' j', sum.inr sum.inl : i' j'] {
Copy link
Member Author

@eric-wieser eric-wieser Feb 26, 2021

Choose a reason for hiding this comment

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

@gebner, this is where I needed propagate_tags from leanprover-community/lean#540. Can you think of a better solution? Should substs just call propagate_tags itself?

Copy link
Member

Choose a reason for hiding this comment

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

Good lord! I think propagate_tags is the least of this proof's problems. Is there any way it can be split up into more manageable and maybe even independently useful chunks?

But yes, substs should call propagate_tags itself.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the first impressions. I don't find it particularly readable when every line is either all_goals or work_on_goal. While I value avoiding duplication, I think it would be better to refactor all_goals { a }, work_on_goal 0 { b }, work_on_goal 1 { b' }, all_goals { c } into { a, b, c }, { a, b', c } here.

I would also introduce a definition for the summand of dom_coprod_fun. This is an unwieldy expression, and you can probably extract two of the big blocks in the proof here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would also introduce a definition for the summand of dom_coprod_fun. This is an unwieldy expression, and you can probably extract two of the big blocks in the proof here.

I've done this as dom_coprod.summand and eliminated dom_coprod_fun - good idea.

Comment on lines 468 to 472
{R : Type*} {M N₁ N₂ : Type*}
[comm_semiring R]
[add_comm_group N₁] [semimodule R N₁]
[add_comm_group N₂] [semimodule R N₂]
[add_comm_monoid M] [semimodule R M]
Copy link
Member

Choose a reason for hiding this comment

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

I would use variables for this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't do this because there is already a global R variable in the file with an associated semiring. It's frustrating that def ... {R : Type*} can shadow variables, but section variables {R : Type*} def ... can't.

I guess I'll just introduce an R' and use that instead...

src/linear_algebra/alternating.lean 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 🎉

bors d+

src/linear_algebra/alternating.lean Outdated Show resolved Hide resolved
src/linear_algebra/alternating.lean Outdated Show resolved Hide resolved
@bors
Copy link

bors bot commented Mar 13, 2021

✌️ eric-wieser can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@bryangingechen
Copy link
Collaborator

Do you think it's worth citing the reference in the PR comment (and adding it to references.bib)?

@@ -281,6 +281,13 @@ @InProceedings{ fuerer-lochbihler-schneider-traytel2020
bibsource = {dblp computer science bibliography, https://dblp.org}
}

@Inproceedings{ Gallier2011Notes,
Copy link
Member Author

Choose a reason for hiding this comment

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

This was what "semantic scholar" suggested, but I have no idea what the right choice is.

Copy link
Collaborator

Choose a reason for hiding this comment

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

LGTM as far as doc-gen is concerned.

@eric-wieser
Copy link
Member Author

@bryangingechen, does the way I've included the reference look ok to you?

@@ -281,6 +281,13 @@ @InProceedings{ fuerer-lochbihler-schneider-traytel2020
bibsource = {dblp computer science bibliography, https://dblp.org}
}

@Inproceedings{ Gallier2011Notes,
Copy link
Collaborator

Choose a reason for hiding this comment

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

LGTM as far as doc-gen is concerned.

docs/references.bib Outdated Show resolved Hide resolved
@eric-wieser
Copy link
Member Author

bors r+

bors bot pushed a commit that referenced this pull request Mar 14, 2021
This implements a variant of the multiplication defined in the second half of [Proposition 22.24 of "Notes on Differential Geometry and Lie Groups" (Jean Gallier)](https://www.cis.upenn.edu/~cis610/diffgeom-n.pdf):

![image](https://user-images.githubusercontent.com/425260/104042315-3dfe7380-51d2-11eb-9b3a-bbbb52d180f0.png)

Despite what the commit generated by bors may claim, none of this is @zhangir-azerbayev's work



Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
@eric-wieser
Copy link
Member Author

bors r-

Commit has the wrong main author, let me try again

@bors
Copy link

bors bot commented Mar 14, 2021

Canceled.

This implements a variant of the multiplication defined in the second half of [Proposition 22.24 of "Notes on Differential Geometry and Lie Groups" (Jean Gallier)](https://www.cis.upenn.edu/~cis610/diffgeom-n.pdf):

![image](https://user-images.githubusercontent.com/425260/104042315-3dfe7380-51d2-11eb-9b3a-bbbb52d180f0.png)
@eric-wieser eric-wieser force-pushed the eric-wieser/alternating_map_product branch from 71d61e3 to 6d7c47b Compare March 14, 2021 10:49
@eric-wieser
Copy link
Member Author

bors r+

bors bot pushed a commit that referenced this pull request Mar 14, 2021
This implements a variant of the multiplication defined in the second half of [Proposition 22.24 of "Notes on Differential Geometry and Lie Groups" (Jean Gallier)](https://www.cis.upenn.edu/~cis610/diffgeom-n.pdf):

![image](https://user-images.githubusercontent.com/425260/104042315-3dfe7380-51d2-11eb-9b3a-bbbb52d180f0.png)
@bors
Copy link

bors bot commented Mar 14, 2021

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat(linear_algebra/alternating): Add dom_coprod [Merged by Bors] - feat(linear_algebra/alternating): Add dom_coprod Mar 14, 2021
@bors bors bot closed this Mar 14, 2021
@bors bors bot deleted the eric-wieser/alternating_map_product branch March 14, 2021 12:22
b-mehta pushed a commit that referenced this pull request Apr 2, 2021
This implements a variant of the multiplication defined in the second half of [Proposition 22.24 of "Notes on Differential Geometry and Lie Groups" (Jean Gallier)](https://www.cis.upenn.edu/~cis610/diffgeom-n.pdf):

![image](https://user-images.githubusercontent.com/425260/104042315-3dfe7380-51d2-11eb-9b3a-bbbb52d180f0.png)
@YaelDillies YaelDillies removed the awaiting-review The author would like community review of the PR label Nov 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants