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:

Status badges

Description (last modified by leif)

Ut oh, the word "monomial" turns out to be ambiguous! There are two definitions!:

http://en.wikipedia.org/wiki/Monomial

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.

  1. Introduce a new method is_term, which returns True for a*x^i*y^j..., i.e. it allows a coefficient. Fortunately, is_term is currently used nowhere in Sage.
  1. 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)

trac_11747.patch (3.9 KB) - added by was 10 years ago.

Download all attachments as: .zip

Change History (9)

Changed 10 years ago by was

comment:1 Changed 10 years ago by was

  • Status changed from new to needs_review

comment:2 Changed 10 years ago by mhansen

Luckily, this choice is consistent with all of the code in sage/combinat/free_module.py .

comment:3 Changed 10 years ago by mhansen

  • Status changed from needs_review to positive_review

Looks good to me.

comment:4 Changed 10 years ago by leif

  • Authors set to William Stein
  • Description modified (diff)
  • Reviewers set to Mike Hansen

comment:5 follow-up: Changed 10 years ago by leif

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 fbissey

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 leif

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 leif

  • Merged in set to sage-4.7.2.alpha3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.