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

provide monomial_coefficients for polynomials and allow single argument for MPolynomialRing_base.monomial #38729

Merged
merged 11 commits into from
Oct 12, 2024

Conversation

mantepse
Copy link
Collaborator

The goal of this pull request is to allow generic programming with instances of CombinatorialFreeModule and polynomial rings.

In particular, we provide an alias monomial_coefficients for the existing method dict of polynomial rings, and we allow to specify the exponents in monomial of multivariate polynomial rings as a single tuple.

Copy link

github-actions bot commented Sep 28, 2024

Documentation preview for this PR (built with commit 92c4646; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@mantepse
Copy link
Collaborator Author

The failure in quantum_clifford is real, I'm looking into it.

@mantepse
Copy link
Collaborator Author

fixed, silly thinko.

@mantepse
Copy link
Collaborator Author

I cannot reproduce the test failure in src/sage/rings/polynomial/polynomial_element.pyx on my computer.

@mantepse
Copy link
Collaborator Author

I thought that ETuples would be tuples, but that is not the case.

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

LGTM.

@mantepse
Copy link
Collaborator Author

mantepse commented Oct 1, 2024

I made another mistake, sorry. There are several places where dict is defined, and I want to make things more generic, not less :-)

@mantepse
Copy link
Collaborator Author

mantepse commented Oct 1, 2024

I think it would actually be good to deprecate the dict method. The name does not make it clear what it does, and it does something very different to the dict function. However, it is probably better to open a separate pull request for that.

@mantepse
Copy link
Collaborator Author

mantepse commented Oct 1, 2024

The plural failure should be unrelated, see #38737

@tscrim
Copy link
Collaborator

tscrim commented Oct 4, 2024

I wouldn't worry too much about aligning dict and monomial_coefficients everywhere. The former is not a category-specified thing.

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

Your dict redirect methods need a doctest though.

@mantepse
Copy link
Collaborator Author

mantepse commented Oct 4, 2024

I wouldn't worry too much about aligning dict and monomial_coefficients everywhere. The former is not a category-specified thing.

Yes. But my goal is to see whether we can make the functionality category-specified, and eventually deprecate dict.

@tscrim
Copy link
Collaborator

tscrim commented Oct 4, 2024

I wouldn't worry too much about aligning dict and monomial_coefficients everywhere. The former is not a category-specified thing.

Yes. But my goal is to see whether we can make the functionality category-specified, and eventually deprecate dict.

I probably would not do that since that method is about the return type of the data for the object rather than something mathematical. I would rather let each implementation handle it as it sees fit.

@mantepse
Copy link
Collaborator Author

mantepse commented Oct 4, 2024

I wouldn't worry too much about aligning dict and monomial_coefficients everywhere. The former is not a category-specified thing.

Yes. But my goal is to see whether we can make the functionality category-specified, and eventually deprecate dict.

I probably would not do that since that method is about the return type of the data for the object rather than something mathematical. I would rather let each implementation handle it as it sees fit.

Yes, I agree.

@mantepse
Copy link
Collaborator Author

mantepse commented Oct 4, 2024

Putting polynomials into a graded category is not desired, if I recall correctly? There is no comment in the code though.

@tscrim
Copy link
Collaborator

tscrim commented Oct 4, 2024

I probably would not do that since that method is about the return type of the data for the object rather than something mathematical. I would rather let each implementation handle it as it sees fit.

Yes, I agree.

I might have misunderstood your intent. Perhaps I should also clarify that I am not in favor of deprecating dict, but perhaps encouraging the use of monomial_coefficients when that is the desired information. It happens to align with the data for, e.g., polynomials, but people might want to actually think of it as a dict. Perhaps that is also a distinction without a difference...

@tscrim
Copy link
Collaborator

tscrim commented Oct 4, 2024

Putting polynomials into a graded category is not desired, if I recall correctly? There is no comment in the code though.

By default, yes, because the morphisms would become too restrictive for most users. What we should do is add an input to specify the grading to each variable, in which case it is clear the user wanted a graded object. (The is what the free algebra does.)

@mantepse
Copy link
Collaborator Author

mantepse commented Oct 4, 2024

Putting polynomials into a graded category is not desired, if I recall correctly? There is no comment in the code though.

By default, yes, because the morphisms would become too restrictive for most users. What we should do is add an input to specify the grading to each variable, in which case it is clear the user wanted a graded object. (The is what the free algebra does.)

I'm very sorry, my question was wrong. It should have been: "Puting polynomials into the category of algebras with basis is not desired, if I recall correctly?"

@tscrim
Copy link
Collaborator

tscrim commented Oct 4, 2024

Putting polynomials into a graded category is not desired, if I recall correctly? There is no comment in the code though.

By default, yes, because the morphisms would become too restrictive for most users. What we should do is add an input to specify the grading to each variable, in which case it is clear the user wanted a graded object. (The is what the free algebra does.)

I'm very sorry, my question was wrong. It should have been: "Puting polynomials into the category of algebras with basis is not desired, if I recall correctly?"

That I don't recall, and I can't see why. They are already implemented as such effectively...

@mantepse
Copy link
Collaborator Author

mantepse commented Oct 4, 2024

Oh, that's good news! I'll try to add _indices and

diff --git a/src/sage/rings/polynomial/polynomial_ring_constructor.py b/src/sage/rings/polynomial/polynomial_ring_constructor.py
index a135fe8de6c..12cceaf4c05 100644
--- a/src/sage/rings/polynomial/polynomial_ring_constructor.py
+++ b/src/sage/rings/polynomial/polynomial_ring_constructor.py
@@ -932,7 +932,7 @@ def polynomial_default_category(base_ring_category, n_variables):
         sage: QQ['s']['t'].category() is UniqueFactorizationDomains() & CommutativeAlgebras(QQ['s'].category()).Infinite()
         True
     """
-    category = Algebras(base_ring_category)
+    category = Algebras(base_ring_category).WithBasis()
 
     if n_variables:
         # here we assume the base ring to be nonzero

@tscrim
Copy link
Collaborator

tscrim commented Oct 4, 2024

In principle, the _indices should be an implementation detail left to the classes, and nothing in the category-provided code should be using it. It’s possible that is not the case though, but the other implementations do not have to follow the CFM design pattern.

@@ -139,7 +139,7 @@ cdef class FreeAlgebraElement_letterplace(AlgebraElement):
sage: sorted(p) # indirect doctest
[((0, 0, 0, 1, 0, 0, 0, 1), 2), ((0, 1, 0, 0, 0, 0, 1, 0), 1)]
"""
cdef dict d = self._poly.dict()
cdef dict d = self._poly.monomial_coefficients()
yield from d.iteritems()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

should I do

Suggested change
yield from d.iteritems()
yield from d.items()

throughout?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be changed since it was part of the Python3 transition.

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM.

@mantepse
Copy link
Collaborator Author

mantepse commented Oct 7, 2024

Thank you :-)

@mantepse
Copy link
Collaborator Author

mantepse commented Oct 8, 2024

Hm, can I set this to "positive review"?

@tscrim
Copy link
Collaborator

tscrim commented Oct 8, 2024

Ah, sorry, I thought I did that.

vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 9, 2024
…llow single argument for MPolynomialRing_base.monomial

    
The goal of this pull request is to allow generic programming with
instances of `CombinatorialFreeModule` and polynomial rings.

In particular, we provide an alias `monomial_coefficients` for the
existing method `dict` of polynomial rings, and we allow to specify the
exponents in `monomial` of multivariate polynomial rings as a single
tuple.
    
URL: sagemath#38729
Reported by: Martin Rubey
Reviewer(s): Frédéric Chapoton, Martin Rubey, Travis Scrimshaw
vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 11, 2024
    
We put polynomials (and friends) into the category of algebras with
basis.

Dependencies: sagemath#38729
    
URL: sagemath#38767
Reported by: Martin Rubey
Reviewer(s): Travis Scrimshaw
@vbraun vbraun merged commit 73874f0 into sagemath:develop Oct 12, 2024
22 of 26 checks passed
@mantepse mantepse deleted the polynomials/monomial_coefficients branch October 13, 2024 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants