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/nat/mul_ind): multiplicative induction principles #8514

Closed
wants to merge 5 commits into from

Conversation

ericrbg
Copy link
Collaborator

@ericrbg ericrbg commented Aug 2, 2021

Co-authored-by: Eric Wieser wieser.eric@gmail.com


Open in Gitpod

whilst writing up the docstring, I realised that the question of uniqueness never really came to mind, which is probably important as these are Sort* valued, instead of just Prop.

@ericrbg ericrbg added the awaiting-review The author would like community review of the PR label Aug 2, 2021
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.

Interestingly, I just proved induction_on_coprime for unique factorization domains. But you can't in good faith return data from that one, since factorization in UFDs is only unique up to units, so I think these recursion principles on ℕ are useful.

Although you might want to generalize to UFD + normalization_domain if you have some time left over... :P

src/data/nat/mul_ind.lean Outdated Show resolved Hide resolved
instance fact.min_fac_prime (n : ℕ) : fact (prime (n + 2).min_fac) :=
⟨min_fac_prime $ succ_succ_ne_one n⟩

-- also not sure where these two should be
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would the div section of data.nat.basic work for this one, or is that too early?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

woo! I found both sides of the implication there so less code ^^

src/data/nat/mul_ind.lean Outdated Show resolved Hide resolved
src/data/nat/mul_ind.lean Outdated Show resolved Hide resolved
src/data/nat/mul_ind.lean Outdated Show resolved Hide resolved
@Vierkantor Vierkantor 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 Aug 11, 2021
@ericrbg
Copy link
Collaborator Author

ericrbg commented Aug 11, 2021

Thanks Anne! As soon as I get a spare bit I'll try see if I can generalize this, I didn't know about normalization_monoid, so hopefully the API there is good enough :) How does the "get any prime" look over there - here I use min_fac, but will I have to do some classical.some faffing?

@Vierkantor
Copy link
Collaborator

Thanks Anne! As soon as I get a spare bit I'll try see if I can generalize this, I didn't know about normalization_monoid, so hopefully the API there is good enough :)

Well, you can try, but if it turns out to be a huge pile of work, please don't spend too much time!

How does the "get any prime" look over there - here I use min_fac, but will I have to do some classical.some faffing?

My approach was to first show the result for a finset.prod of powers of pairwise coprime prime elements, and then translate the multiset of factors to a finset that satisfies the assumptions. The difficult part is choosing everything uniquely, because here I do it up to associated.

@ericrbg
Copy link
Collaborator Author

ericrbg commented Aug 16, 2021

I tried a little with normalization_monoid, and quickly grew overwhelmed, so in fact I'll leave it as a note to future work. I'm planning to work on this stuff a bit more (refactoring padic_val_prime to hopefully require far less imports, too), but for now I think it'd be nice to have these in, and hopefully work on them later! (I'll put the note in a bit, I have a sneaky feeling this build is going to fail...)

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 still can't think of a good alternative name for rec_on_coprime', so I made a Zulip thread.

LGTM otherwise.

src/data/nat/mul_ind.lean Outdated Show resolved Hide resolved
Co-authored-by: Anne Baanen <Vierkantor@users.noreply.github.com>
@ericrbg ericrbg 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 Aug 21, 2021
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 agree with @eric-wieser's suggestions in the Zulip thread. LGTM otherwise, so:

bors d+

src/data/nat/mul_ind.lean Outdated Show resolved Hide resolved
src/data/nat/mul_ind.lean Outdated Show resolved Hide resolved
@bors
Copy link

bors bot commented Aug 23, 2021

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

@github-actions github-actions bot added delegated The PR author may merge after reviewing final suggestions. and removed awaiting-review The author would like community review of the PR labels Aug 23, 2021
@ericrbg
Copy link
Collaborator Author

ericrbg commented Aug 23, 2021

bors r+

bors bot pushed a commit that referenced this pull request Aug 23, 2021
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>



Co-authored-by: Eric <37984851+ericrbg@users.noreply.github.com>
@bors
Copy link

bors bot commented Aug 23, 2021

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat(data/nat/mul_ind): multiplicative induction principles [Merged by Bors] - feat(data/nat/mul_ind): multiplicative induction principles Aug 23, 2021
@bors bors bot closed this Aug 23, 2021
@bors bors bot deleted the mul_ind branch August 23, 2021 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
delegated The PR author may merge after reviewing final suggestions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants