Ticket #2460 (closed defect: fixed)

Opened 10 months ago

Last modified 2 months ago

some issues with factorization.py

Reported by: was Assigned to: was
Priority: major Milestone: sage-3.1.2
Component: basic arithmetic Keywords: editor_wstein
Cc:

Description

Various people worked on factorization.py and unfortunately ignored some implicit assumptions in what that code is supposed to do. In particular, this function 

    def base_ring(self):
        if len(self) > 0:
            return self[0][0].parent()
        else:
            return self.unit().parent()

assumes that (1) ever element has the same parent, and (2) the parent is a ring. Neither assumption need be satisfied.

This is_commutative function then relies on base_ring working. Here's an example of this leading to *wrong* answers: 

sage: R.<x,y> = FreeAlgebra(QQ,2)
sage: Factorization([(3,1), (x,2), (y,3), (x,1), (y,2)])
3 * x^3 * y^5

Proposal: Simply call Sequence on the list of bases in the factorization to get a new list where the basis lie in a common university. Then refine is_commutative to mean that the universe is a commuative ring, and only then commute factors automatically.

Second, after the above is resolved, the sort function for comparison should call universe() (not base_ring) and use some sensible defaults, before resorting to that mess of code in the current sort method.

Attachments

trac_2460.patch (3.8 kB) - added by gfurnish on 03/10/2008 09:29:04 AM.

Change History

03/10/2008 09:10:24 AM changed by was

  • owner changed from somebody to was.
  • status changed from new to assigned.

03/10/2008 09:22:25 AM changed by gfurnish

  • owner changed from was to gfurnish.
  • status changed from assigned to new.

03/10/2008 09:22:34 AM changed by gfurnish

  • status changed from new to assigned.

03/10/2008 09:29:04 AM changed by gfurnish

  • attachment trac_2460.patch added.

03/10/2008 09:29:45 AM changed by gfurnish

  • summary changed from some issues with factorization.py to [with patch, needs review] some issues with factorization.py.

03/10/2008 09:33:57 AM changed by gfurnish

There is a possibility that fixing commutativity may other things.

03/10/2008 10:12:58 AM changed by gfurnish

  • owner changed from gfurnish to was.
  • status changed from assigned to new.

There are non-trivial issues involved with fixing this (namely, moving things to the universe causes issues with repr and commutes, and I can't find a way to fix those issues without refactoring other code to make this work well, so this should probably see some discussion.

03/10/2008 10:14:54 AM changed by gfurnish

  • summary changed from [with patch, needs review] some issues with factorization.py to some issues with factorization.py.

03/22/2008 02:25:08 PM changed by mabshoff

  • summary changed from some issues with factorization.py to [with patch, do not review] some issues with factorization.py.

06/19/2008 09:46:32 PM changed by craigcitro

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

06/22/2008 10:56:22 PM changed by mabshoff

  • milestone changed from sage-3.1.1 to sage-duplicate/invalid.

Hi Carig,

not to be prickly Pete, but can give a reason why this was invalidated?

Cheers,

Michael

06/27/2008 01:20:25 PM changed by was

  • status changed from closed to reopened.
  • resolution deleted.

Woah -- I incorrectly thought this had long since been fixed. NOT. 

sage: R.<x,y> = FreeAlgebra(QQ,2)
sage: sage: Factorization([(3,1), (x,2), (y,3), (x,1), (y,2)])
3 * x^3 * y^5

06/27/2008 01:20:32 PM changed by was

  • summary changed from [with patch, do not review] some issues with factorization.py to [with patch, needs review] some issues with factorization.py.

07/03/2008 12:08:33 AM changed by mabshoff

  • milestone changed from sage-duplicate/invalid to sage-3.1.1.

Ok, moving this back to a current milestone so that it can be seen :)

Cheers,

Michael

07/06/2008 04:02:33 AM changed by mabshoff

  • keywords set to editor_wstein.

William,

can you be the editor of this patch? Feel free to bounce it back to me.

Cheers,

Michael

08/22/2008 10:35:50 AM changed by cremona

Looking at factorization.py I was all ready to fix all the problems I could see -- using Sequence to get a common universe for the bases on construction, cache this base_ring, only allow operations between factorizations with the same base_ring, and so on.

But then I saw what appeared to be a totally weird example: 

sage: F = Factorization([(ZZ^3, 2), (ZZ^2, 5)], cr=True); F
(Ambient free module of rank 2 over the principal ideal domain Integer Ring)^5 * 
(Ambient free module of rank 3 over the principal ideal domain Integer Ring)^2

This bears no relation at all to what I thought the Factorization class was for. Doing a search_src showed that this is designed in to support splitting of modular symbols spaces (and similar).

This leaves a question almost certainly for William: is it really sensible to have one class serve both as the structure to hold "prime factorizations" for UFDs and other rings, as well as to hold lists of subspaces with multiplicities?

If so, perhaps we need to refactor this to have a base class which just handles the basics, with (at least) 2 derived classes, one for rings factorizations and one for additive decompositions?

John

# I have added this posting to trac#2460 too.

08/23/2008 09:09:43 AM changed by cremona

  • summary changed from [with patch, needs review] some issues with factorization.py to some issues with factorization.py.

I think the issues raised here have all been dealt with by the patches I put up at #3927 (which started out as a separate enhancement, hence the new ticket). In particular the good parts of the patch attached to this ticket have been used there.

I suggest that this ticket be closed, with a link to #3927 instead.

11/14/2008 12:52:38 AM changed by mhansen

  • status changed from reopened to closed.
  • resolution set to fixed.
  • milestone changed from sage-3.2.1 to sage-3.1.2.

I think that this can be closed as well due to #3927.