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

feat(data/pnat): factor finsupps #3291

Closed
wants to merge 10 commits into from
Closed

feat(data/pnat): factor finsupps #3291

wants to merge 10 commits into from

Conversation

awainverse
Copy link
Collaborator

@awainverse awainverse commented Jul 5, 2020

adds a new definition of factorization for pnats, using finsupps
proves facts about order isomorphisms of lattices


@awainverse awainverse added the awaiting-review The author would like community review of the PR label Jul 5, 2020
@awainverse
Copy link
Collaborator Author

I have added everything to one new file, although it should probably be split up. I've left comments on the sections that I think should be moved. Probably the section on lattice operations under order_embeddings and order_isos should be moved to order.order_iso, and the section on lattice instances on finsupps in particular should be put either in data.finsupp or a new file called data.finsupp.order or data.finsupp.lattice.

@jcommelin jcommelin changed the title feat(data/pnat) factor finsupps feat(data/pnat): factor finsupps Jul 8, 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.

Some trivial comments. More to come.

src/data/pnat/factor_finsupps.lean Show resolved Hide resolved
src/data/pnat/factor_finsupps.lean Outdated Show resolved Hide resolved
src/data/pnat/factor_finsupps.lean Outdated Show resolved Hide resolved
src/data/pnat/factor_finsupps.lean Outdated Show resolved Hide resolved
@awainverse awainverse 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 awaiting-review The author would like community review of the PR labels Jul 9, 2020
@awainverse awainverse changed the title feat(data/pnat): factor finsupps feat(data/pnat): factor finsupps (deps #3335) Jul 9, 2020
@awainverse awainverse added awaiting-review The author would like community review of the PR 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 Jul 25, 2020
@semorrison
Copy link
Collaborator

Could you write a short account, either in a comment here or ideally as a module doc-string, about what improvement this makes over existing machinery in mathlib? I have to admit I'm not certain what the purpose of this is yet.

@sgouezel sgouezel 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 25, 2020
@bryangingechen bryangingechen changed the title feat(data/pnat): factor finsupps (deps #3335) feat(data/pnat): factor finsupps Jul 25, 2020
@awainverse
Copy link
Collaborator Author

I've added some of the definitions/results that I think work better for finsupps than for multisets to the module docstring. I see some bad formatting from writing this when I had spent about half as much time with lean as I have now, so I'll try to clean that up after I can download fresh oleans from the server.

@awainverse
Copy link
Collaborator Author

Another more speculative advantage of finsupps for factorization is that you could use enat-valued finsupps to provide the same definition of unique factorization for general monoids and monoid_with_zero, but I'm not doing that in this PR, and I'm not sure how useful that really is.

@awainverse awainverse 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 30, 2020
@semorrison
Copy link
Collaborator

semorrison commented Aug 4, 2020

Sorry, @robertylewis, I'm not sure I can usefully review this. I'm imagining the intended application of this work is to make it easier to develop the theory of multiplicative functions in number theory, but I'm no expert in that direction so can't judge how well this serves that purpose.

@kbuzzard or @Vierkantor, maybe you could take a look?

@semorrison semorrison requested review from Vierkantor and removed request for semorrison August 4, 2020 11:49
@robertylewis
Copy link
Member

Oh, no problem, I had just assigned you because you were the last to comment.

@Vierkantor
Copy link
Collaborator

I have not tried out the code in too much detail, but here are my impressions about the general approach:

  • finsupp is a strict generalization of multiset. A priori, such a generalization can be useful (my suggestion would be to use exponents in to represent the factors of positive rational numbers).
  • Setting the value of a finsupp at one point does seem to be easier than changing the multiplicity of a multiset element, and that operation is useful for number theory.
  • If we generalize from multiset to finsupp, I don't see the purpose of keeping around the multiset-based lemmas. Especially since data.pnat.factors doesn't seem to be imported by anything in mathlib. I know re-proving the multiset-based lemmas is not fun, but it should be a clearer demonstration of the power (or awkwardness?) of the finsupp-based approach.
  • I have more experience with unique_factorization_domain, which also uses a multiset of factors. In fact, there seems to be a lot of duplication between the two, so it might be worth defining a factor_finsupp for a general semiring/monoid_with_zero(?), then prove that these exist for both and unique factorization domains.
  • finsupp has some API issues, but it's better to fix the issues than avoid using finsupp.

So my preliminary conclusion is that using a finsupp to count the prime factors of a number is probably a good idea, but I'd like to see it happen for more algebraic structures than just natural numbers so we can decrease the amount of duplication, rather than increase it.

@awainverse
Copy link
Collaborator Author

@Vierkantor, I like the sound of everything you've said.
I've started a Zulip thread https://leanprover.zulipchat.com/#narrow/stream/116395-maths/topic/Refactoring.20UFDs.20to.20Monoids to discuss a possible generalization.

@robertylewis
Copy link
Member

What's the status here? I get the sense from the conversation that this isn't "request review" right now. @awainverse @Vierkantor ?

@semorrison semorrison removed their assignment Aug 15, 2020
@semorrison semorrison added WIP Work in progress and removed awaiting-review The author would like community review of the PR labels Aug 15, 2020
@semorrison
Copy link
Collaborator

Perhaps it makes sense to close this while other work is going on?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants