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/{finsupp,dfinsupp}/basic): Scalar actions with zero #12880

Closed
wants to merge 2 commits into from

Conversation

YaelDillies
Copy link
Collaborator

@YaelDillies YaelDillies commented Mar 22, 2022

Add a few scalar instances on finsupp and dfinsupp assuming smul_with_zero and stronger rather than distrib_mul_action.


Open in Gitpod

@YaelDillies YaelDillies added the awaiting-review The author would like community review of the PR label Mar 22, 2022
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.

Is it worth copying these to monoid_algebra, add_monoid_algebra, polynomial, and mv_polynomial?

@eric-wieser
Copy link
Member

eric-wieser commented Mar 22, 2022

This all feels a bit unpleasant to me - either:

  1. we care about smul_with_zero, and do want these instances. I really don't think we want to duplicate every single piece of the finsupp / dfinsupp smul API for it, and I think we should instead add the lower level typeclass you suggest
  2. we don't care about smul_with_zero, and should just leave these instances missing
  3. we care more about your smul_with_zero_on_one_side than we do the current smul_with_zero, and should just change the definition

I would lean towards option 2 for now, since adding yet another typeclass in the action hierarchy is hard to justify without motivating examples.

@eric-wieser eric-wieser 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 Mar 23, 2022
@adomani
Copy link
Collaborator

adomani commented Mar 26, 2022

This is all happening at a level somewhat below the one where I am comfortable: I always try to stay away from explicitly working with finsupps and have learned to move around in polynomial, where the implementation is sufficiently hidden.

If this PR means that when I reach for simp to try to solve an finsupp.sum arising from a polynomial computation, it will simplify more, then I like this! Otherwise, I do not think that I have an opinion about these changes.

I am sorry if this review is vague, but as I said, I feel that I never would like to worry about finsupps, except that I suffer through them in polynomials!

@eric-wieser
Copy link
Member

eric-wieser commented Mar 26, 2022

@adomani, can you remind us what the original motivation for smul_with_zero was? Specifically, for what X were you interested in smul_with_zero X M?

@adomani
Copy link
Collaborator

adomani commented Mar 26, 2022

I originally wanted smul_with_zero for something coming from LTE. The specific issue was dealing with a vector space over the reals, and ℕ-submodules spanned by (finite) sets of vectors. The typeclass was supposed to make it easy to convert between multiplications by ℕ, ℤ, ℚ, ℝ on the same underlying vector space.

The idea was to combine this with the fact that multiplication by non-zero elements is injective (and hence the is_smul_regular definition as well).

Possibly, by now, there are better ways to deal with this?

@eric-wieser
Copy link
Member

Maybe the simple answer is just "smul_with_zero exists such that mul_zero_class can be expressed with too". I guess that was probably useful for linking regular and smul_regular.

That's enough to convince me that my option 3 above is a bad idea

@mathlib-dependent-issues-bot mathlib-dependent-issues-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 Aug 18, 2022
@mathlib-dependent-issues-bot mathlib-dependent-issues-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 Sep 23, 2022
@mathlib-dependent-issues-bot
Copy link
Collaborator

@YaelDillies
Copy link
Collaborator Author

A version of this made it to master.

@YaelDillies YaelDillies deleted the finsupp_scalar branch October 23, 2022 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-author A reviewer has asked the author a question or requested changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants