Opened 8 years ago

Last modified 5 years ago

#15904 new defect

Defining __eq__ without defining __ne__ or __cmp__: sage/combinat

Reported by: darij Owned by:
Priority: major Milestone: sage-6.4
Component: combinatorics Keywords: sage-combinat, equality, inheritance
Cc: tscrim, sage-combinat, nthiery, jakobkroeker Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps: wrongAnswerMarker

Status badges

Description (last modified by darij)

I think one should always define at least one of __ne__ and __cmp__ on a class when one defines __eq__, unless one really wants to have != to behave differently from the negation of == (or one is happy with the __ne__ inherited from the superclass; but then why would one redefine __eq__ to begin with?):

sage: ContreTableaux(4) == ContreTableaux(4)
True
sage: ContreTableaux(4) != ContreTableaux(4)
True

or also (here the != is inherited from CombinatorialObject):

sage: Core([1,1],3) != Core([1,1],4)
False
sage: Core([1,1],3) == Core([1,1],4)
False

Here is a result of searching for this antipattern in the sage/combinat subfolder: https://www.dropbox.com/s/lca1yfw1qsz6ycy/ne.txt Some of these do define __cmp__, but redefining __ne__ might lead to a speedup, so I kept them in the file even if these are not bugs per se.

Should we just fix them all robotically?

Change History (11)

comment:1 Changed 8 years ago by darij

  • Description modified (diff)

comment:2 Changed 8 years ago by nthiery

Thanks for investigating this! It's certainly annoying.

In principle TestSuite? includes some checks about this. So it would be interesting to see whether TestSuite? was run on those failing examples.

Whenever possible, I would vote for including a trivial implementation of __ne__ by negation of __eq__ as high up in the class hierarchy (e.g. CombinatorialObject) to minimize the change and make it more likely for future classes to not fall in the trap.

Cheers,

Nicolas

Last edited 8 years ago by nthiery (previous) (diff)

comment:3 follow-up: Changed 8 years ago by darij

I'm not sure if TestSuite? is always applicable in such cases -- e.g. what should I apply TestSuite? to in order to see the issue with Core?

Redefining CombinatorialObject.__ne__ might come with a little speed penalty, but I guess it translates into either a bugfix or a speedup in most classes inheriting from CombinatorialObject because those redefine __eq__ to something either more correct or faster. This wouldn't, however, solve all the issues here. For example, ContreTableaux only inherits from Parent (BTW: why does ContreTableaux.category() raise a TypeError??), while CombinatorialSpecies inherits from SageObject, etc.

comment:4 in reply to: ↑ 3 Changed 8 years ago by nthiery

Replying to darij:

I'm not sure if TestSuite? is always applicable in such cases -- e.g. what should I apply TestSuite? to in order to see the issue with Core?

On the parent. The methods _test_elements_eq and friends from the Sets.ParentMethods? category does this kind of checks, and it will be interesting to see if the heuristics there are enough to catch some or all of what you detected. If not, it can be a good occasion to invent new heuristics :-)

Redefining CombinatorialObject.__ne__ might come with a little speed penalty, but I guess it translates into either a bugfix or a speedup in most classes inheriting from CombinatorialObject because those redefine __eq__ to something either more correct or faster.

+1.

This wouldn't, however, solve all the issues here. For example, ContreTableaux only inherits from Parent (BTW: why does ContreTableaux.category() raise a TypeError??), while CombinatorialSpecies inherits from SageObject, etc.

Yup. I have always been of the opinion that SageObject? should really be handling this for us; but if I recall correctly there are some Python versus Cython classes issues, since the protocol varies slightly from one to the other.

Cheers,

Nicolas

comment:5 follow-up: Changed 8 years ago by darij

Hmm, there actually *is* a noticeable amount of CombinatorialObjects? which redefine neither __eq__ nor __ne__ (tableaux, permutations, Dyck words, partition tuples, ...), so we might want to check for speed regressions. But they'll probably be insubstantial (for most combinatorial operations, there should be no need to check for inequality).

Where can I learn how to use clonable lists?

About TestSuite?: what am I doing wrong?

sage: c = Core([1,1],3)
sage: c
[1, 1]
sage: parent(c)
3-Cores of length 2
sage: parent(c).TestSuite()
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-4-7ca0b34ac3ec> in <module>()
----> 1 parent(c).TestSuite()

/scratch/sage-6.1.1/local/lib/python2.7/site-packages/sage/structure/parent.so in sage.structure.parent.Parent.__getattr__ (sage/structure/parent.c:6997)()

/scratch/sage-6.1.1/local/lib/python2.7/site-packages/sage/structure/misc.so in sage.structure.misc.getattr_from_other_class (sage/structure/misc.c:1606)()

AttributeError: 'Cores_length_with_category' object has no attribute 'TestSuite'

comment:6 in reply to: ↑ 5 Changed 8 years ago by nthiery

Replying to darij:

Where can I learn how to use clonable lists?

sage.structure.list_clone?

also available as:

http://www.sagemath.org/doc/reference/structure/sage/structure/list_clone.html

See in particular the demo examples.

About TestSuite?: what am I doing wrong?

sage: c = Core([1,1],3)
sage: c
[1, 1]
sage: parent(c)
3-Cores of length 2
sage: parent(c).TestSuite()

TestSuite(parent(c)).run()

Cheers,

Nicolas

comment:7 Changed 8 years ago by darij

Also outside sage/combinat:

sage: Q = QuadraticForm(ZZ, 3, [1,2,3,4,5,6])
sage: R = QuadraticForm(ZZ, 3, [1,2,3,4,5,6])
sage: Q == R
True
sage: Q != R
True

comment:8 Changed 8 years ago by darij

What do you think about fixing all known cases by hand (fast and kinda urgent) before trying any general approaches?

comment:9 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:10 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:11 Changed 5 years ago by jakobkroeker

  • Cc jakobkroeker added
  • Stopgaps set to wrongAnswerMarker
Note: See TracTickets for help on using tickets.