Opened 7 years ago
Last modified 4 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 |
Description (last modified by )
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 7 years ago by
- Description modified (diff)
comment:2 Changed 7 years ago by
comment:3 follow-up: ↓ 4 Changed 7 years ago by
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 7 years ago by
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 fromCombinatorialObject
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 fromParent
(BTW: why doesContreTableaux.category()
raise a TypeError??), whileCombinatorialSpecies
inherits fromSageObject
, 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: ↓ 6 Changed 7 years ago by
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 7 years ago by
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 7 years ago by
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 7 years ago by
What do you think about fixing all known cases by hand (fast and kinda urgent) before trying any general approaches?
comment:9 Changed 7 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:10 Changed 7 years ago by
- Milestone changed from sage-6.3 to sage-6.4
comment:11 Changed 4 years ago by
- Cc jakobkroeker added
- Stopgaps set to wrongAnswerMarker
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,