Ticket #12706 (closed defect: fixed)
monomials of 0
| Reported by: | vferay | Owned by: | sage-combinat |
|---|---|---|---|
| Priority: | major | Milestone: | sage-5.2 |
| Component: | commutative algebra | Keywords: | polynomial, zero |
| Cc: | Work issues: | ||
| Report Upstream: | N/A | Reviewers: | Keshav Kini |
| Authors: | Frédéric Chapoton | Merged in: | sage-5.2.beta1 |
| Dependencies: | Stopgaps: |
Description (last modified by vferay) (diff)
The monomials method applied to the null polynomials returns [0].
According to me, it should return [ ]. This can be problematic because when we ask the coefficients of the monomial 0, an error is raised (see below).
If nobody objects, I will write a patch to correct this.
sage: QX=PolynomialRing(QQ,'x',2) sage: P=QX(0) sage: P.monomials() [0] sage: [P.coefficient(mon) for mon in P.monomials()] TypeError: degrees must be a monomial
Attachments
-
trac_12706_monomials_of_zero-fc.patch
(6.4 KB) -
added by chapoton 11 months ago.
Change History
comment:4 Changed 14 months ago by nthiery
Hi Valentin!
On the principle, I totally agree that the result should be the empty list. And this would be consistent with CombinatorialFreeModule?.
However the feature was documented and this change is backward incompatible. So I guess this requires a call for votes on sage-devel.
Another thing to be careful with is that there are several implementations of monomials::
sage: search_def("monomials")
combinat/free_module.py:589: def monomials(self):
rings/multi_power_series_ring_element.py:950: def monomials(self):
rings/quotient_ring_element.py:658: def monomials(self):
rings/monomials.py:27:def monomials(v, n):
rings/polynomial/pbori.pyx:3657: def monomials(self):
rings/polynomial/multi_polynomial_sequence.py:480: def monomials(self):
rings/polynomial/multi_polynomial_sequence.py:497: def nmonomials(self):
rings/polynomial/multi_polynomial_libsingular.pyx:3271: def monomials(self):
rings/polynomial/plural.pyx:2387: def monomials(self):
rings/polynomial/multi_polynomial_element.py:979: def monomials(self):
crypto/mq/mpolynomialsystem.py:394: def monomials(self):
crypto/mq/mpolynomialsystem.py:863: def monomials(self):
All the above should be checked and fixed (when needed) at once.
Thanks for volunteering on cleaning up work!
Cheers,
Nicolas
comment:5 Changed 11 months ago by chapoton
Here is a ticket, that works.
It seems that changing the behaviour of 0.monomials() could trigger some problems in the is_homogeneous method in other places. I have found and corrected one. Maybe there are some others. Let us see the tests by the patchbot.
Could somebody ask the question on sage-devel for the backward incomp. change ?
comment:7 Changed 11 months ago by chapoton
- Keywords polynomial, zero added
- Authors changed from vferay to Frédéric Chapoton
comment:8 Changed 11 months ago by chapoton
- Component changed from combinatorics to commutative algebra
comment:9 Changed 11 months ago by chapoton
- Milestone changed from sage-5.1 to sage-5.2
Ok, tests seems to be ok, as the bot is green. Could somebody please review ?
comment:10 Changed 11 months ago by kini
- Status changed from needs_review to positive_review
- Reviewers set to Keshav Kini
It looks good to me!
comment:11 Changed 10 months ago by chapoton
As this is a (small) backward incompatible change, a vote has been opened here :
https://groups.google.com/d/topic/sage-devel/jqzOxgfQItE/discussion
So far, everybody has voted in favor of the change. Maybe one can consider that the change is approved ?
comment:12 Changed 10 months ago by kini
I don't think a vote was even necessary, but sure (IMO).
comment:13 Changed 10 months ago by jdemeyer
- Status changed from positive_review to closed
- Resolution set to fixed
- Merged in set to sage-5.2.beta1
