Opened 7 years ago

Last modified 21 months ago

#13186 needs_info defect

Equality of Factorizations

Reported by: caruso Owned by: tbd
Priority: minor Milestone: sage-6.4
Component: factorization Keywords: equality factorization
Cc: caruso Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:


Currently, two Factorization object are considered to be equal if they have the same value, but a Factorization object is not equal to its value:

sage: P.<x> = QQ[]
sage: a = x+1; b = x+2; c = x+3
sage: F = Factorization([(a,1),(b,1),(c,1)]); F
(x + 1) * (x + 2) * (x + 3)
sage: F1 = Factorization([(a*b,1),(c,1)]); F1
(x + 3) * (x^2 + 3*x + 2)
sage: F2 = Factorization([(a,1),(b*c,1)]); F2
(x + 1) * (x^2 + 5*x + 6)
sage: F == F1
sage: F == F2
sage: F1 == F2
sage: F == a*b*c

Is this normal? (It seems to me really weird.)

PS: Am I supposed to ask this kind of questions here or is there a better place?

Attachments (1)

trac_13186_v1.patch (2.7 KB) - added by chapoton 6 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 7 years ago by caruso

  • Cc caruso added

comment:2 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:3 Changed 6 years ago by chapoton

The methods __eq__ and __ne__ seems not to be implemented in the class.

comment:4 Changed 6 years ago by caruso

I would happily implement them but I'm wondering what should be the correct behaviour of these methods. Do you have an opinion? (Don't forget that a Factorization object can be "non commutative" as well.)

comment:5 Changed 6 years ago by chapoton

well, one could try the following in the comparison of self and other

1) look if other is in self.parent (or maybe do that for the first factors)

2) if not compare (the value of self) and other

3) if yes, ask if self.parent is_commutative

4) if yes, sort and compare ; if not compare without sorting ?

Changed 6 years ago by chapoton

comment:6 Changed 6 years ago by chapoton

here is a patch with a proposal for __eq__

comment:7 Changed 6 years ago by caruso

The patch looks ok to me (but I also think that I'm not a good reviewer).

I'm just wondering whether we really want this:

sage: factor(691*(x-4)*(x+6)) == factor(691*(y-4)*(y+6)) 

Indeed, even if 691 is a unit in one case and is not in the other case, the factorizations are the same, aren't they?

comment:8 Changed 6 years ago by chapoton

Well, one must choose the meaning of equality.

One precise meaning would be

  • a factorisation is made of a unit and a set/list of factors
  • two factorisations are equal if they have the same unit and the same factors (up to order maybe)

Another possible solution would be to consider the unit as just any other factor. I do no like this solution, and I prefer the previous (current) behavior

If you want, you can maybe ask for opinions on sage-devel.

Something else: I have just thought that it would sometimes be good to separate factors in the center of the ring (commuting) and other factors, and to implement "partially commutative factorisations". But this is something for the wishlist.

comment:9 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:10 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:11 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:12 Changed 5 years ago by chapoton

  • Branch set to u/chapoton/13186
  • Commit set to 84b36a2056754713bc05056b28e7bb9ab86b0f24
  • Status changed from new to needs_review

New commits:

84b36a2trac 13186 comparison of factorisations

comment:13 Changed 5 years ago by vdelecroix

  • Status changed from needs_review to needs_work


Please add specifications to the doc! It is not clear to me (and to you as well looking at comment:8) what should be an equality between element.

I really found weird the begining

if not isinstance(other, Factorization):
    return self.value() == other

how can you compare a factorization with something else. Would you like that

sage: [1,2,3] == (1,2,3)

Instead of using Sequence, what about

sage: from sage.structure.element import get_coercion_model
sage: cm = get_coercion_model()
sage: cm.common_parent(1,1/2)
Rational field

(not so important, as at the end of the day the same code is called)

If the ring is commutative your test is wrong as the elements of the underlying universe are not necessarily sortable... you can not assume that the order is the same based on the fact that you call sorted. It might also depend on the specification you will add...

You need to define a __ne__ if you want that != works as well. Basically it would be

def __ne__(self, other):
    return not (self == other)


comment:14 Changed 5 years ago by nbruin

There are other problems too:

sage: R.<x>=QQ[]
sage: hash(x^2)
sage: hash(factor(x^2))
sage: factor(x^2).value()==x^2

If you want to equate factorizations with their values (a reasonable thing to do) then their hashes should be equal too. I'd think a reasonable thing to do is to set

factor(A) == B iff factor(A).value() == B
hash(factor(A)) == hash(factor(A).value())

and in fact to implement them by punting to the tests on values.

Note that if "factorizations" are ever returned in domains that do not have unique factorizations, this might be confusing: after all, there are supposed to be factorizations of the same value into irreducibles there that are not mutually equal.

comment:15 Changed 21 months ago by chapoton

  • Branch u/chapoton/13186 deleted
  • Commit 84b36a2056754713bc05056b28e7bb9ab86b0f24 deleted
  • Status changed from needs_work to needs_info

There is now a __richcmp__ method in this class. One needs to check if the issue still stands.

Last edited 21 months ago by chapoton (previous) (diff)
Note: See TracTickets for help on using tickets.