Ticket #3927 (closed enhancement: fixed)

Opened 3 months ago

Last modified 3 months ago

[with new patch, positive review] Several enhancements and bug fixes for Factorization class

Reported by: cremona Assigned to: somebody
Priority: minor Milestone: sage-3.1.2
Component: basic arithmetic Keywords: factorization
Cc:

Description

This works: 

sage: factor(10)*factor(15)^(-1)             
2 * 3^-1

and so does this: 

sage: factor(10/15)        
2 * 3^-1

but not this: 

sage: factor(10)/factor(15)     
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)

/home/john/sage-3.1.test/spkg/build/python-2.5.2.p3/tmp/<ipython console> in <module>()

TypeError: unsupported operand type(s) for /: 'Factorization' and 'Factorization'

So: Factorizations can be multiplied and inverted but not divided, which is a bit silly. I suggest adding a __div___() method.

Attachments

sage-trac3927.patch (2.4 kB) - added by cremona on 08/22/2008 08:26:51 AM.
sage-trac3927a.patch (2.6 kB) - added by cremona on 08/22/2008 08:27:10 AM.
sage-trac3927b.patch (11.3 kB) - added by cremona on 08/23/2008 09:04:42 AM.
trac3927-fix-gcd-lcm.patch (1.5 kB) - added by cwitty on 08/23/2008 04:31:32 PM.

Change History

08/22/2008 08:26:51 AM changed by cremona

  • attachment sage-trac3927.patch added.

08/22/2008 08:27:10 AM changed by cremona

  • attachment sage-trac3927a.patch added.

08/22/2008 08:34:09 AM changed by cremona

  • summary changed from Factorization class has no division implemented to [with patches, needs review] Factorization class has no division implemented.

Two patches: the first implements division, the second implements gcd() and lcm() for Factorizations. The first also fixes a small gap discovered while testing those, namely that for FreeAlgebras?, the element 1 could not be inverted. Now, in any ring, for an element a for which a.is_unit() is true, a.invert() returns self. (For many rings, 1 is the only element for which .is_unit() does not return a NotImplementedError?).

Theses patches are essentially orthogonal to the other one #2460 concerning factorization.py. They are based on 3.1.1.

John

08/22/2008 11:54:21 AM changed by cremona

  • milestone set to sage-3.1.2.

08/23/2008 09:04:42 AM changed by cremona

  • attachment sage-trac3927b.patch added.

08/23/2008 09:06:34 AM changed by cremona

  • summary changed from [with patches, needs review] Factorization class has no division implemented to [with new patch, needs review] Several enhancements and bug fixes for Factorization class.

The third patch deals with the issues left from trac #2460. Each Factorization now has a universe (cached as attribute self.__universe) computed using the Sequence idea proposed in trac #2460. The base_ring() function has been changed to universe() and returns the universe. If no universe is found it just sets it to None (for example, this is the case for Factorizations of modular symbol spaces).

I also added a new function is_integral and changed the docstrings to reflect the fact that the exponents needs not be positive (for example, factor(2/3) has always worked and returned a prime factorization with a negative exponent).

Since Factorization is used in a lot of different places I did -testall before posting this, and dealt with a few minor things which arose (no-one minded base_ring() being renamed universe(), but in 2 or 3 places unit_part() was changed to unit()).

All three patches should be applied in order; based on 3.1.1.

I think the add and sub methods are totally pointless but have left them in for now.

08/23/2008 04:31:19 PM changed by cwitty

  • summary changed from [with new patch, needs review] Several enhancements and bug fixes for Factorization class to [with new patch, positive review] Several enhancements and bug fixes for Factorization class.

I fixed a few issues with gcd and lcm, but everything else looks good.

Positive review. (Apply all four patches.)

08/23/2008 04:31:32 PM changed by cwitty

  • attachment trac3927-fix-gcd-lcm.patch added.

08/24/2008 01:54:39 AM changed by cremona

Last patch is fine -- thanks.

08/24/2008 07:59:58 PM changed by mabshoff

  • status changed from new to closed.
  • resolution set to fixed.

Merged sage-trac3927.patch, sage-trac3927a.patch, sage-trac3927b.patch and trac3927-fix-gcd-lcm.patch in Sage 3.1.2.alpha1