Opened 10 years ago
Closed 10 years ago
#11747 closed defect (fixed)
is_monomial and is_term
Reported by: | was | Owned by: | AlexGhitza |
---|---|---|---|
Priority: | minor | Milestone: | sage-4.7.2 |
Component: | algebra | Keywords: | sd32 |
Cc: | Merged in: | sage-4.7.2.alpha3 | |
Authors: | William Stein | Reviewers: | Mike Hansen |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
Ut oh, the word "monomial" turns out to be ambiguous! There are two definitions!:
And in Sage we evidently have *randomly* (?) and inconsistenly chosen between the two definitions, which is very unfortunate.
sage: R.<x> =QQ[] sage: (2*x).is_monomial() # definition 2 in univariate poly True sage: (x).is_monomial() True sage: R.<x,y> =QQ[] sage: (2*x).is_monomial() # definition 1 in multivariate poly False sage: x.is_monomial() True
Etc. Fortunately, is_monomial()
is called in only about 5 or 6 places in the entire Sage source library, according to search_src('is_monomial')
.
Reading the argument in Wikipedia further, and discussing this with Tom Boothby, we've decided the following would work for us.
- Introduce a new method
is_term
, which returns True fora*x^i*y^j...
, i.e. it allows a coefficient. Fortunately,is_term
is currently used nowhere in Sage.
- Unify
is_monomial
to require the coefficient to be 1. This means changing univariate polynomials to be consistent with multivariate polynomials.
Apply trac_11747.patch to the Sage library.
Attachments (1)
Change History (9)
Changed 10 years ago by
comment:1 Changed 10 years ago by
- Status changed from new to needs_review
comment:2 Changed 10 years ago by
comment:3 Changed 10 years ago by
- Status changed from needs_review to positive_review
Looks good to me.
comment:4 Changed 10 years ago by
- Description modified (diff)
- Reviewers set to Mike Hansen
comment:5 follow-up: ↓ 7 Changed 10 years ago by
Any clue what can cause the following?
sage -t -long -force_lib "devel/sage/sage/rings/polynomial/multi_polynomial_element.py" ********************************************************************** File "/tmp/Sage/release/sage-4.7.2.alpha2/devel/sage/sage/rings/polynomial/multi_polynomial_element.py", line 101: sage: x + QQbar.random_element() # indirect doctest Expected: x - 2 Got: x + 4 ********************************************************************** 1 items had failures: 1 of 5 in __main__.example_2 ***Test Failed*** 1 failures.
This doesn't happen with only this patch applied, and no other patch (merged so far) touches sage/rings/polynomial/multi_polynomial_element.py
(nor qqbar.py
).
No single of the other patches merged with this one seems to cause this doctest error alone, i.e., apparently only a combination.
(And it's not #10635.)
comment:6 Changed 10 years ago by
One thing that strikes me is that the call is for QQbar.random_element(). It seems to me that unless there is a good reason for it, the fact that we always had the same result regardless of architecture is just blind luck.
So was there ever a justification why it should be "-2"?
comment:7 in reply to: ↑ 5 Changed 10 years ago by
Replying to leif:
Any clue what can cause the following? [...]
Oh, my bad; must have been blind or confused something when trying to track down another error.
The doctest failure most probably originates from the patch at #11725, so this ticket is certainly the wrong one to report on.
Sorry for the noise.
comment:8 Changed 10 years ago by
- Merged in set to sage-4.7.2.alpha3
- Resolution set to fixed
- Status changed from positive_review to closed
Luckily, this choice is consistent with all of the code in sage/combinat/free_module.py .