Opened 6 years ago
Closed 4 years ago
#22029 closed enhancement (fixed)
Element richcmp: never use id()
Reported by:  Marc Mezzarobba  Owned by:  

Priority:  major  Milestone:  sage8.7 
Component:  coercion  Keywords:  python3, richcmp 
Cc:  Frédéric Chapoton  Merged in:  
Authors:  Marc Mezzarobba, Jeroen Demeyer  Reviewers:  Julian Rüth, Marc Mezzarobba 
Report Upstream:  N/A  Work issues:  
Branch:  e311ab1 (Commits, GitHub, GitLab)  Commit:  e311ab134f9d0c16dbbb800d35ba2c4325b4beb2 
Dependencies:  #27241  Stopgaps: 
Description (last modified by )
As discussed in the sagedevel thread starting with <o21nte$6jp$1@blaine.gmane.org>
(https://groups.google.com/d/msg/sagedevel/YVFdxPH6avk/4OZUmzLHBgAJ),
coercion_model.richcmp()
should not fall back on comparing by type/id.
This branch implements comparisons for Element
the same way as in Python 3: a TypeError
is raised for uncomparable objects (instead of comparing by id
).
Dependencies: #22297, #22344, #22346, #22369, #22370, #22371, #22372, #22373, #22376, #22382, #24968, #26917, #26931, #26933, #26934, #26935, #26936, #26937, #26938, #26947, #27003, #27009, #27010, #27026, #27027, #27029, #27123, #27241
Change History (152)
comment:2 Changed 6 years ago by
Description:  modified (diff) 

comment:3 Changed 6 years ago by
Description:  modified (diff) 

comment:4 followup: 6 Changed 6 years ago by
Replying to jdemeyer:
Do you plan to work on this? If yes, fill in your name as reviewer.
as author? owner?
If no, let me know.
I've started doing experiments, but I'll most likely need help from people who understand the parts of Sage that rely on the current behavior. At the very least I'll try to push a first draft in the next few days.
comment:5 followups: 9 15 Changed 6 years ago by
My main concern right now is that sage.graphs
heavily relies on sorting lists of vertices, while various other parts of Sage create graphs with vertices that mix Elements with other Python objects.
An option might be to try again with key=id
when sort()
fails, but (with no global understanding of the code, at least) there's a major risk of getting inconsistent orders with different subsets of vertices.
Another idea would be to do introduce utilities like
# universal_cmp.pyx cpdef lt(x, y): try: return x < y except TypeError: if type(x) is type(y): return id(x) < id(y) else: return type(x) < type(y) ... cdef class key(object): def __init__(self): self.value = value def __lt__(self, other): return lt(self.value, other.value) ...
and then use universal_cmp.lt()
and sort(key=universal_cmp.key)
, but this won't work either when one of the types involved only has a partial order, since this order will not be consistent with id
.
Yet another variant would be to pass a comparison function (or, equivalently but probably better in view of the transition to python3, a key
as above) that wraps cmp()
and catches TypeError
s. Not really ideal either, but at least it wouldn't introduce any regression(?), so perhaps that would be enough for this ticket...
Any advice?
comment:6 Changed 6 years ago by
Replying to mmezzarobba:
Replying to jdemeyer:
Do you plan to work on this? If yes, fill in your name as reviewer.
as author? owner?
Sorry, I meant as author.
comment:7 followup: 8 Changed 6 years ago by
Authors:  → Marc Mezzarobba 

By the way, why did you remove the reference to the sagedevel thread?
comment:8 followup: 10 Changed 6 years ago by
Replying to mmezzarobba:
By the way, why did you remove the reference to the sagedevel thread?
Because I didn't understand how to interpret it. A hyperlink would be better.
comment:9 followup: 11 Changed 6 years ago by
Replying to mmezzarobba:
My main concern right now is that
sage.graphs
heavily relies on sorting lists of vertices
Why does it rely on sorting vertices?
comment:10 Changed 6 years ago by
Replying to jdemeyer:
Replying to mmezzarobba:
By the way, why did you remove the reference to the sagedevel thread?
Because I didn't understand how to interpret it. A hyperlink would be better.
I personally prefer MessageIds, as they don't depend on a particular archive...
comment:11 followup: 13 Changed 6 years ago by
Replying to jdemeyer:
Replying to mmezzarobba:
My main concern right now is that
sage.graphs
heavily relies on sorting lists of verticesWhy does it rely on sorting vertices?
In sparse_graph
and generic_graph
, mostly. Some of the occurrences look like they are there for algorithmic purposes, others to make the output more readable, some both at the same time... I didn't look closely enough to say more.
comment:12 Changed 6 years ago by
Description:  modified (diff) 

comment:13 followup: 14 Changed 6 years ago by
Replying to mmezzarobba:
In
sparse_graph
andgeneric_graph
, mostly. Some of the occurrences look like they are there for algorithmic purposes, others to make the output more readable, some both at the same time... I didn't look closely enough to say more.
It's also important to note the difference between oldstyle cmp()
and newstyle rich comparisons. Ideally, this branch would only affect the latter.
comment:14 Changed 6 years ago by
Replying to jdemeyer:
It's also important to note the difference between oldstyle
cmp()
and newstyle rich comparisons. Ideally, this branch would only affect the latter.
I'm not sure I follow you: cmp()
can end up calling Element.__richcmp__()
, and that's perhaps the main reason why getting rid of the comparison by id there is not trivial.
comment:15 Changed 6 years ago by
Replying to mmezzarobba:
My main concern right now is that
sage.graphs
heavily relies on sorting lists of vertices, while various other parts of Sage create graphs with vertices that mix Elements with other Python objects.An option might be to try again with
key=id
whensort()
fails, but (with no global understanding of the code, at least) there's a major risk of getting inconsistent orders with different subsets of vertices.
If you have no information on the objects you're sorting, I don't think there is anything that can save you. If your objects are guaranteed to be hashable you could try sorting on the hash, but that will leave your results undefined when hashes clash. In any case, if you rely on "first try what the object implements and then use a fallback if that fails", you're doomed.
In fact, if you allow your vertices to be labelled with arbitrary objects, you're already losing transitivity of equality in sage, via the standard
sage: a=GF(3)(1); b =31; c=GF(5)(1) sage: a==b,b==c,a==c (True, True, False)
It seems that labels get mangled anyway, though, so I think them not being sorted properly is only a minor concern by comparison (sorry):
sage: a=GF(3)(1); b =31; c=GF(5)(1) sage: L=[[a,b,c],[a,c,b],[b,a,c],[b,c,a],[c,a,b],[c,b,a]] sage: for l in L: ....: G=Graph() ....: for i in l: ....: G.add_vertex(i) ....: print [parent(j) for j in G.vertices()] ....: [<type 'int'>, Integer Ring] [<type 'int'>, Integer Ring] [<type 'int'>, Integer Ring] [<type 'int'>, Integer Ring] [<type 'int'>, Integer Ring] [<type 'int'>, Integer Ring]
It seems to me the only sane approaches are to not rely on sorting (in python it seems being hashable is more ubiquitous than being sortable, so fast lookup is likely better accomplished via hash functions), or to declare that the labels are sortable (making failure a user error).
This stuff causes real problems: since the old python interface for __cmp__
methods required one to provide nonerrors for "<" and ">" testing if one wants to implement "==", sage is riddled with apparent implementations of orderings that are in actuality inconsistent. This has cause problems in at least one case, where labels in published tables of elliptic curves were based on nonsensical but assumedconsistent ordering of number field elements:
https://groups.google.com/forum/#!topic/sagent/aP5LP09sADY
This has convinced me that the only sane approach is to deprecate "cmp" style interfaces as quickly as possible and instead rely on "richcmp", with errors for ordering if not obviously correct.
comment:16 Changed 6 years ago by
Branch:  → public/ticket/22029richcmp_fallback 

Commit:  → 749dfbe13d7de1a0d520a9eddfd843b8354d8ad6 
Status:  new → needs_review 
Here is a first attempt. All (standard, long) tests should pass unless I made a lastminute mistake. See the commit messages for some comments. I'm not 100% happy with the changes, but I don't think I can do much better in a reasonable amount of time, feel free to commit improvements to the existing branch!
Last 10 new commits:
d45cd53  CGraphBackend: fix a segfault when given a nonexisting vertex

6e0e1b2  Fix use of comparison in simplicial_complex

2925d0f  Bug in Poset.is_slender(?)

2b0bcc9  Some benign doctest failures related to #22029

5f36421  Adhoc comparison functions for graph vertices

6d728fa  {c,sparse}_graph: systematically turn integerlike vertices into ints

fcda2c2  graph_plot: sort by id

127a11d  remove or fix some meaningless doctests for cmp()

29fe1f8  misc fixes after changes to coercion_model.richcmp

749dfbe  misc fixes after changes to coercion_model.richcmp

comment:17 followups: 19 24 37 Changed 6 years ago by
Status:  needs_review → needs_work 

I am a strong 1 on the removal of the !=
tests and the changes of foo != bar
to (the fugly) not (foo == bar)
.
The failures you have in rigged_configurations.py
is because you moved the definition of case_S
. You have also made the cases in the code more difficult to follow. Please revert those changes.
comment:18 followup: 20 Changed 6 years ago by
This is bad:
# (Note that, when y is not a Sage Element, we may have ended up here # from a call to cmp(). In this case, and in this case only, it may be # better to emulate what Python does and compare by type/id. But we # have no way(?) to distinguish this situation from the "normal" case # of a comparison operator.)
comment:19 Changed 6 years ago by
Replying to tscrim:
I am a strong 1 on the removal of the
!=
tests and the changes offoo != bar
to (the fugly)not (foo == bar)
.
Ugly, I agree, but safer in generic code (because of cases where equality is not decidable, like expressions, or where not enough information is available, like with intervals)... And code that might end up comparing objects of different type is generic code.
The failures you have in
rigged_configurations.py
is because you moved the definition ofcase_S
. You have also made the cases in the code more difficult to follow. Please revert those changes.
I might do it, but I'd like first to understand in detail the alternative you are suggesting, and to see some evidence of a consensus that it is a better option.
In any case, please feel free to commit improvements yourself, and I'll do my best to review them.
comment:20 followup: 21 Changed 6 years ago by
Replying to jdemeyer:
This is bad:
# (Note that, when y is not a Sage Element, we may have ended up here # from a call to cmp(). In this case, and in this case only, it may be # better to emulate what Python does and compare by type/id. But we # have no way(?) to distinguish this situation from the "normal" case # of a comparison operator.)
What do you mean? If I understand right, this is an issue that will solve itself if sage ever switches to Python3...
comment:21 followup: 22 Changed 6 years ago by
Replying to mmezzarobba:
If I understand right, this is an issue that will solve itself if sage ever switches to Python3...
Given that we currently still have Python 2, that's irrelevant.
comment:22 Changed 6 years ago by
Replying to jdemeyer:
Replying to mmezzarobba:
If I understand right, this is an issue that will solve itself if sage ever switches to Python3...
Given that we currently still have Python 2, that's irrelevant.
Sorry, I fear I still don't understand your comment. The problem with cmp()
was the main thing I asked about in the sagedevel post that led to this ticket, and you basically replied that it was okay from your point of view to remove the fallback code from richcmp
. Now it looks to me like you are saying the contrary.
comment:23 Changed 6 years ago by
Commit:  749dfbe13d7de1a0d520a9eddfd843b8354d8ad6 → fb9beb5d953a78e7799b5b6f63fe31d21d4abb31 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
fb9beb5  misc fixes after changes to coercion_model.richcmp

comment:24 Changed 6 years ago by
Replying to tscrim:
The failures you have in
rigged_configurations.py
is because you moved the definition ofcase_S
. You have also made the cases in the code more difficult to follow.
Fixed, sorry for the stupid mistake. That's all I did for now. It wouldn't be hard to revert the other changes related to !=
, but I'm waiting for answers to the followup question I just asked on sagedevel.
comment:25 Changed 6 years ago by
Commit:  fb9beb5d953a78e7799b5b6f63fe31d21d4abb31 → 6e696450f6e118da8add1d9c33d99797e9a5234d 

Branch pushed to git repo; I updated commit sha1. New commits:
6e69645  Elt richcmp: return NotImplemented for nonElements...

comment:26 followup: 27 Changed 6 years ago by
Another take, now returning NotImplemented
more often. Due to Jeroen's doubts, I left out the idea of treating SageObject
s in a special way. I do however handle types such as int
and float
like Element
, since this is what the coercion framework usually does.
This should make it possible to revert some of my other changes. I haven't done it yet.
comment:27 followup: 28 Changed 6 years ago by
Replying to mmezzarobba:
I do however handle types such as
int
andfloat
likeElement
since this is what the coercion framework usually does.
Yes, the coercion framework tries to coerce those types to a Sage Element
. However, it doesn't do anything special in the case that such a coercion fails. So I don't see why you should treat the failedcoercionwithint
orfloat
case differently. Just return NotImplemented
in that case.
comment:28 followup: 29 Changed 6 years ago by
Replying to jdemeyer:
Yes, the coercion framework tries to coerce those types to a Sage
Element
. However, it doesn't do anything special in the case that such a coercion fails.
But the net effect is that arithmetic that mixes Element
s and objects of these types will succeed or fail (with an error coming from the coercion framework) depending whether there is a common parent or not.
So I don't see why you should treat the failedcoercionwith
int
orfloat
case differently. Just returnNotImplemented
in that case.
Do you really want things like
sage: GF(2)(1) > float(0.) True sage: RBF(0) < float(1.) True sage: SymmetricGroup(2)(()) != int(1) True
even though it takes exactly one LOC to prevent them and analogous code with Sage Element
s raises an error (so that, in particular, the behavior will be different for preparsed and pure Python code)?
comment:29 Changed 6 years ago by
Replying to mmezzarobba:
Do you really want things like
sage: GF(2)(1) > float(0.) True sage: RBF(0) < float(1.) True sage: SymmetricGroup(2)(()) != int(1) True
In Python 2? Yes, I want that.
In Python 3? No, I do not want that.
comment:30 followup: 31 Changed 6 years ago by
In other words: just follow the common Python convention which specifies that a rich comparison (not __cmp__
) should return NotImplemented
when it cannot decide a comparison.
The fact that we directly raise a TypeError
for Sage Elements is merely a shortcut to get the correct behaviour and to get a nicer error message referring to the parents.
comment:31 followup: 32 Changed 6 years ago by
Replying to jdemeyer:
In other words: just follow the common Python convention which specifies that a rich comparison (not
__cmp__
) should returnNotImplemented
when it cannot decide a comparison.
I tend to see the situation we are talking about not as one where it can't decide, but as one where it can decide that the comparison doesn't make sense. And not to find “just follow the Python[2] convention” such a strong argument when Sage already breaks it in much worse ways...
comment:32 followup: 33 Changed 6 years ago by
Replying to mmezzarobba:
And not to find “just follow the Python[2] convention” such a strong argument when Sage already breaks it in much worse ways...
How is it relevant whether or not Sage breaks certain Python conventions in much worse ways?
If Python specifies how to do arithmetic/comparisons, why not follow that?
comment:33 Changed 6 years ago by
Replying to jdemeyer:
How is it relevant whether or not Sage breaks certain Python conventions in much worse ways?
When you start deviating from the general conventions, “your” objects already don't interact with the rest of the system as other developers will expect, and your job IMHO is to minimize the damage.
Now, there is no doubt that you have orders of magnitude more experience of Sage development than I'll likely ever do, so I guess I'll follow your advice even without understanding it if that's really the only way. But I'd really prefer (in general: I'm bringing this up because it is not the first time I have trouble understanding your comments) if you could explain why you think a certain technical choice is better instead of just stating that it is.
comment:34 Changed 6 years ago by
Commit:  6e696450f6e118da8add1d9c33d99797e9a5234d → a5a1e2b76332d136936c7cbe94cf42f93ec6203d 

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
af1a94a  Fix use of comparison in simplicial_complex

744942c  Bug in Poset.is_slender(?)

ea1910e  Some benign doctest failures related to #22029

c87d7e3  Adhoc comparison functions for graph vertices

107ad68  {c,sparse}_graph: systematically turn integerlike vertices into ints

fd03cd8  graph_plot: sort by id

69cc074  remove or fix some meaningless doctests for cmp()

b33f2eb  Elt richcmp: return NotImplemented for nonElements...

87ca195  misc fixes after recent changes to coercion_model.richcmp()

a5a1e2b  Jeroen doesn't like this

comment:35 followup: 40 Changed 6 years ago by
Status:  needs_work → needs_review 

New version of the complete patch series. Tested without the last commit, not fully with it.
I probably won't have much time to work on this in the coming 68 weeks, please feel free to take over if you have improvements to make.
comment:37 Changed 6 years ago by
Replying to tscrim:
I am a strong 1 on the removal of the
!=
tests and the changes offoo != bar
to (the fugly)not (foo == bar)
.
Does the new version (that returns NotImplemented
in more cases, so illtyped comparisons by !=
will typically succeed when the other member is not an Element
) address your concerns?
comment:38 Changed 6 years ago by
Status:  needs_review → needs_work 

comment:39 Changed 6 years ago by
Commit:  a5a1e2b76332d136936c7cbe94cf42f93ec6203d → 87ca195bbec9a5b2d7e6aafc872f7cdd0867f7f8 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
comment:40 Changed 6 years ago by
Replying to mmezzarobba:
New version of the complete patch series. Tested without the last commit, not fully with it.
Turns out there where test failures with that commit. Since it is still not clear to me that it is really what we want, I undid it for now.
comment:41 Changed 6 years ago by
Status:  needs_work → needs_review 

comment:42 Changed 6 years ago by
Cc:  Frédéric Chapoton added 

Keywords:  cmp added 
comment:43 Changed 6 years ago by
Reviewers:  → Julian Rüth 

I also have trouble seeing why what jdemeyer proposes would be better. For me the proposed changes look fine.
comment:44 Changed 6 years ago by
Status:  needs_review → needs_work 

It is plausible that the adhoc comparison functions for graph vertices can be simplified (or even removed) thanks to the fact that the general comparison function now returns NotImplemented
for nonElements
. I haven't had time to check yet.
Also, there are apparently test failures with sage2.6...
I'll try to work on these issues in 12 weeks unless someone beats me to it.
comment:45 Changed 6 years ago by
Commit:  87ca195bbec9a5b2d7e6aafc872f7cdd0867f7f8 → ea99124e08f6b438793a35cc99ad4a4f809764ba 

Branch pushed to git repo; I updated commit sha1. New commits:
ea99124  Merge branch 'public/ticket/22029richcmp_fallback' in 7.6.b2

comment:47 Changed 6 years ago by
Summary:  A step toward safer comparisons → Element comparisons: never use id() 

Let me try to "reboot" this ticket with a different approach: when coercion fails, assume that elements are different (so ==
and !=
would not give an error).
comment:48 Changed 6 years ago by
Summary:  Element comparisons: never use id() → Element richcmp: never use id() 

comment:49 Changed 6 years ago by
Description:  modified (diff) 

comment:50 Changed 6 years ago by
The good pieces of the current branch should be split up into different tickets. It is doing way too much for one ticket.
comment:51 Changed 6 years ago by
I might tackle Parent
(#22344) first. It is a similar but hopefully easier ticket.
comment:52 Changed 6 years ago by
Dependencies:  → #22342, #22344, #22346, #22347 

comment:53 followups: 55 60 Changed 6 years ago by
Dependencies:  #22342, #22344, #22346, #22347 

I hope to have time tomorrow to clean up the branch a little. Then you can do whatever you want with it. But I still think that raising an error when coercion fails and both objects to be compared are Elements is a better tradeoff.
comment:54 Changed 6 years ago by
Dependencies:  → #22342, #22344, #22346, #22347 

comment:55 Changed 6 years ago by
Replying to mmezzarobba:
I hope to have time tomorrow to clean up the branch a little. Then you can do whatever you want with it. But I still think that raising an error when coercion fails and both objects to be compared are Elements is a better tradeoff.
I (also) think in that case we are safe to conclude that ==
and !=
should be False
and True
, respectively. The coercion framework does not allow us to create a new coercion path after that point IIRC. For <
and >
, raising an error is consistent with Python (specifically a TypeError
).
comment:56 Changed 6 years ago by
Dependencies:  #22342, #22344, #22346, #22347 → #22342, #22344, #22346, #22347, #22348 

comment:57 Changed 6 years ago by
Dependencies:  #22342, #22344, #22346, #22347, #22348 → #22342, #22344, #22346, #22347, #22348, #22349 

comment:58 Changed 6 years ago by
Dependencies:  #22342, #22344, #22346, #22347, #22348, #22349 → #22342, #22344, #22346, #22347, #22349 

comment:59 Changed 6 years ago by
Commit:  ea99124e08f6b438793a35cc99ad4a4f809764ba → e58fc9637c9e96b8900bb52b4a72f827a7820e82 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
e58fc96  Element richcmp: never use id()

comment:60 followup: 61 Changed 6 years ago by
Replying to mmezzarobba:
I hope to have time tomorrow to clean up the branch a little.
It's not really about cleaning the branch, it's about identifying pieces which make sense to be a new ticket.
comment:61 followup: 65 Changed 6 years ago by
Branch:  public/ticket/22029richcmp_fallback → trac/u/mmezzarobba/22029richcmp 

Commit:  e58fc9637c9e96b8900bb52b4a72f827a7820e82 
Here is a version that does not require #22349 (nor other intrusive changes to graphs/
) nor #22346 and passes all (short) tests but one even if we make a != b
fail when there is no coercion. Which is what the branch currently does, but that shouldn't be hard to change. (Jeroen, I didn't update the dependency list because I don't really known what you have in mind with this ticket; the true dependencies with the current branch are the one that are obvious from the commits.)
Pretty much everything is improvable, especially the “main” commit, but I don't think I have left anything too dirty.
The one test that fails is:
File "src/sage/combinat/integer_vector_weighted.py", line 272, in sage.combinat.integer_vector_weighted.WeightedIntegerVectors_all.__init__ Failed example: TestSuite(C).run() Expected nothing Got: Failure in _test_elements_neq: Traceback (most recent call last): File "/home/marc/co/sage/local/lib/python2.7/sitepackages/sage/misc/sage_unittest.py", line 293, in run test_method(tester = tester) File "/home/marc/co/sage/local/lib/python2.7/sitepackages/sage/categories/sets_cat.py", line 1330, in _test_elements_neq tester.assertNotEqual(x == y, x != y, File "sage/structure/element.pyx", line 1125, in sage.structure.element.Element.__richcmp__ (/home/marc/co/sage/src/build/cythonized/sage/structure/element.c:10379) return coercion_model.richcmp(self, other, op) File "sage/structure/coerce.pyx", line 1907, in sage.structure.coerce.CoercionModel_cache_maps.richcmp (/home/marc/co/sage/src/build/cythonized/sage/structure/coerce.c:20514) raise bin_op_exception(_operator_to_string(op), x, y) TypeError: unsupported operand parent(s) for !=: 'Integer vectors of 0 weighted by [2, 1, 3]' and 'Integer vectors of 1 weighted by [2, 1, 3]'  The following tests failed: _test_elements_neq
Replying to jdemeyer:
It's not really about cleaning the branch, it's about identifying pieces which make sense to be a new ticket.
I'm not sure I understand the difference. Anyway, all commits except for the last two are intended to be independent. I didn't test them individually, though, and some probably shouldn't be merged separately without adding a test or two. Please feel free to move anything you want to separate tickets.
comment:62 Changed 6 years ago by
Branch:  trac/u/mmezzarobba/22029richcmp → u/mmezzarobba/22029richcmp 

Commit:  → 3842d12db9c2ae70dfd38acbed0002a2f6cd0b71 
Last 10 new commits:
e779b2c  FGP_Element: cmp > richcmp

a5fe7fe  linear_expression: cmp > richcmp

bff3a10  CGraphBackend: fix a segfault when given a nonexisting vertex

3c6c86f  Fix use of comparison in simplicial_complex

6662d83  Bug in Poset.is_slender(?)

83af733  {c,sparse}_graph: systematically turn integerlike vertices into ints

5225368  graph_plot: sort by id

580e2fb  remove or fix some meaningless doctests for cmp()

9136fa2  Don't richcompare Elements by type/id

3842d12  Additional changes to allow element != obj to raise an error

comment:63 Changed 6 years ago by
Apparently the failure in integer_vector_weighted
is due to the fact that DisjointUnionEnumeratedSets(facade=True)
are parents that can construct elements whose parents differ from the parent that constructed them:
sage: V = WeightedIntegerVectors([2,1,3]) sage: V.an_element().parent() is V False
I don't know what to do to fix the issue. Should there be a coercion from WeightedIntegerVectors(n, w)
to WeightedIntegerVectors_all(w)
? (It may not be easy to set up one, since as far as I understand it is currently not possible to create elements with elt.parent() == WeightedIntegerVectors_all(w)
using normal mechanisms, one would have to construct them manually.) Should DisjointUnionEnumeratedSets
override the test suite? Should WeightedIntegerVectors
use facade=False
?...
comment:64 Changed 6 years ago by
For example, the following patch seems to solve the issue, but it is a bit too complicated and fragile to my taste. Do you think it is worth spending time adding docstrings?

src/sage/combinat/integer_vector_weighted.py
a b AUTHORS: 16 16 #***************************************************************************** 17 17 from __future__ import print_function, absolute_import 18 18 19 from sage.structure.coerce_maps import CallableConvertMap 19 20 from sage.structure.unique_representation import UniqueRepresentation 20 21 from sage.structure.parent import Parent 22 from sage.categories.pushout import ConstructionFunctor, IdentityConstructionFunctor 23 from sage.categories.enumerated_sets import EnumeratedSets 21 24 from sage.categories.finite_enumerated_sets import FiniteEnumeratedSets 22 25 from sage.categories.infinite_enumerated_sets import InfiniteEnumeratedSets 23 26 from sage.categories.sets_with_grading import SetsWithGrading … … from sage.combinat.integer_vector import IntegerVector 28 31 from sage.combinat.words.word import Word 29 32 from sage.combinat.permutation import Permutation 30 33 34 class WeightedIntegerVectorFunctor(ConstructionFunctor): 35 rank = 11 36 coercion_reversed = True 37 def __init__(self, n): 38 self.n = n 39 ConstructionFunctor.__init__(self, EnumeratedSets(), EnumeratedSets()) 40 def _apply_functor(self, all_vectors): 41 return WeightedIntegerVectors(self.n, all_vectors._weights) 42 def __eq__(self, other): 43 return (isinstance(other, WeightedIntegerVectorFunctor) 44 and other.n == self.n) 45 def merge(self, other): 46 if isinstance(other, WeightedIntegerVectorFunctor): 47 return IdentityConstructionFunctor() 31 48 32 49 class WeightedIntegerVectors(Parent, UniqueRepresentation): 33 50 r""" … … class WeightedIntegerVectors(Parent, UniqueRepresentation): 111 128 self._n = n 112 129 self._weights = weight 113 130 Parent.__init__(self, category=FiniteEnumeratedSets()) 131 all_vectors = WeightedIntegerVectors_all(self._weights) 132 emb = CallableConvertMap(self, all_vectors, IntegerVector) 133 self._populate_coercion_lists_(embedding=emb) 114 134 115 135 Element = IntegerVector 116 136 137 def construction(self): 138 return (WeightedIntegerVectorFunctor(self._n), 139 WeightedIntegerVectors_all(self._weights)) 140 117 141 def _element_constructor_(self, lst): 118 142 """ 119 143 Construct an element of ``self`` from ``lst``.
comment:65 followup: 68 Changed 6 years ago by
comment:66 followups: 67 69 Changed 6 years ago by
I would say this is a bigger underlying problem with DisjointUnionEnumeratedSets
. This comes from its doc:
sage: class MyUnion(DisjointUnionEnumeratedSets): ....: def __init__(self): ....: DisjointUnionEnumeratedSets.__init__(self, ....: Family([1,2], Permutations)) sage: pp = MyUnion() sage: pp.list() [[1], [1, 2], [2, 1]]
We have
sage: pp.list()[0].parent() Standard permutations of 1
I suspect the biggest problem comes from the fact it does pass the facade option to the category framework. I think your solution is too specific and somewhat overengineered. While I think there should be an easy mechanism for allowing coercions when we have a nonfacade parent, that seems to be tangent to the problem at hand.
The fact that this is causing a problem with a !=
test is worrisome to me too that there is something subtle not caught by your branch. In particular, you can have !=
that results in an error, which I strongly believe should never happen.
I also disagree with the change in !=
test; we should at least make the DummyParent
compare by id so the test remains valid.
comment:67 followup: 70 Changed 6 years ago by
Thanks for your commments.
Replying to tscrim:
I suspect the biggest problem comes from the fact it does pass the facade option to the category framework. I think your solution is too specific and somewhat overengineered.
I agree :)
. But I don't know what else to do.
The fact that this is causing a problem with a
!=
test is worrisome to me too that there is something subtle not caught by your branch. In particular, you can have!=
that results in an error, which I strongly believe should never happen.
Yes, I know we disagree about that. I kept this behavior for now to see how much trouble it would cause, but I can live with a variant that never fails...
I also disagree with the change in
!=
test; we should at least make theDummyParent
compare by id so the test remains valid.
I don't see which change you are talking about, sorry.
comment:68 followup: 71 Changed 6 years ago by
comment:69 Changed 6 years ago by
Replying to tscrim:
I suspect the biggest problem comes from the fact it does pass the facade option to the category framework. I think your solution is too specific and somewhat overengineered. While I think there should be an easy mechanism for allowing coercions when we have a nonfacade parent, that seems to be tangent to the problem at hand.
The fact that this is causing a problem with a
!=
test is worrisome to me too that there is something subtle not caught by your branch. In particular, you can have!=
that results in an error, which I strongly believe should never happen.
I forgot to ask: are weighted integer vectors intended to be comparable by <
etc.? If not, i.e. if the only issue is comparison by !=
, and if we agree that !=
should never fail, then we can indeed ignore the problem for this ticket.
comment:70 Changed 6 years ago by
Replying to mmezzarobba:
Thanks for your commments.
Replying to tscrim:
I suspect the biggest problem comes from the fact it does pass the facade option to the category framework. I think your solution is too specific and somewhat overengineered.
I agree
:)
. But I don't know what else to do.
I have an idea. I should be able to fix it tomorrow (which I will do on a separate ticket).
The fact that this is causing a problem with a
!=
test is worrisome to me too that there is something subtle not caught by your branch. In particular, you can have!=
that results in an error, which I strongly believe should never happen.Yes, I know we disagree about that. I kept this behavior for now to see how much trouble it would cause, but I can live with a variant that never fails...
Okay, I wasn't sure about on a quick skimover what behavior you were trying to support.
I also disagree with the change in
!=
test; we should at least make theDummyParent
compare by id so the test remains valid.I don't see which change you are talking about, sorry.

src/sage/structure/element_wrapper.pyx
diff git a/src/sage/structure/element_wrapper.pyx b/src/sage/structure/element_wrapper.pyx index c24a33c..801f524 100644
a b cdef class ElementWrapper(Element): 327 327 sage: l11 != l12 328 328 True 329 329 sage: l11 != l21 330 True 330 Traceback (most recent call last): 331 ... 332 TypeError: unsupported operand parent(s) for !=: 'A parent' and 333 'Another parent' 331 334 332 335 Testing less than:: 333 336
I believe this fails because the DummyParent
doesn't have comparisons.
Replying to mmezzarobba:
I forgot to ask: are weighted integer vectors intended to be comparable by < etc.? If not, i.e. if the only issue is comparison by !=, and if we agree that != should never fail, then we can indeed ignore the problem for this ticket.
It should be lex ordering, which should be inherited from ClonableArray
.
comment:71 Changed 6 years ago by
Replying to mmezzarobba:
Replying to jdemeyer:
Please set the tickets that you want reviewed to needs_review.
Done, thanks.
Could you also remove those commits from the branch on this ticket?
comment:72 followup: 74 Changed 6 years ago by
The fundamental difference between my branch and your branch is that my branch does not fall back to __cmp__
if rich comparison fails. Given that __cmp__
will go away anyway in Python 3, I strongly disagree with relying on __cmp__
for rich comparisons. That will make it even harder to get rid of Python 2 __cmp__
comparisons.
comment:73 Changed 6 years ago by
Dependencies:  #22342, #22344, #22346, #22347, #22349 

Description:  modified (diff) 
Not sure what are real dependencies and what not, so I'm just listing all potentially relevant tickets in the ticket description.
comment:74 followup: 76 Changed 6 years ago by
Replying to jdemeyer:
The fundamental difference between my branch and your branch is that my branch does not fall back to
__cmp__
if rich comparison fails. Given that__cmp__
will go away anyway in Python 3, I strongly disagree with relying on__cmp__
for rich comparisons. That will make it even harder to get rid of Python 2__cmp__
comparisons.
I fear I don't understand what you mean. If you're talking about the fact that my version of richcmp can return NotImplemented
in some cases, that's something I added following your suggestion on sagedevel...
comment:75 Changed 6 years ago by
Commit:  3842d12db9c2ae70dfd38acbed0002a2f6cd0b71 → 33e88b0c7b9d4ce464fd55da6725751b478f7662 

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
9ac003d  Fix use of comparison in simplicial_complex

4796945  Bug in Poset.is_slender(?)

983e660  graph_plot: sort by id

30d9fb0  remove or fix some meaningless doctests for cmp()

6d2d496  #22376: additional change at Travis' request

61feee1  {c,sparse}_graph: systematically turn integerlike vertices into ints

918a0cb  linear_expression: cmp > richcmp

7a240cc  Merge dependencies

8530b05  Don't richcompare Elements by type/id

33e88b0  Additional changes to allow element != obj to raise an error

comment:76 followup: 78 Changed 6 years ago by
Replying to mmezzarobba:
If you're talking about the fact that my version of richcmp can return
NotImplemented
in some cases
Yes, I am talking about that. When Python sees NotImplemented
, it will call __cmp__
.
that's something I added following your suggestion on sagedevel...
Well, I don't remember what you refer to, but that was not such a good suggestion then...
comment:77 Changed 6 years ago by
Description:  modified (diff) 

The DisjointUnionEnumeratedSets
issue is now #22382.
comment:78 followup: 80 Changed 6 years ago by
Replying to jdemeyer:
that's something I added following your suggestion on sagedevel...
Well, I don't remember what you refer to, but that was not such a good suggestion then...
<585262E2.3060709@cage.ugent.be>
/ https://groups.google.com/d/msg/sagedevel/YVFdxPH6avk/4OZUmzLHBgAJ
comment:79 Changed 6 years ago by
Description:  modified (diff) 

comment:80 Changed 6 years ago by
Replying to mmezzarobba:
<585262E2.3060709@cage.ugent.be>
/ https://groups.google.com/d/msg/sagedevel/YVFdxPH6avk/4OZUmzLHBgAJ
Right. What I said there makes sense in principle in a world where only Python 2 exists.
I did not think of the Python 3 porting in that thread. Given that we are moving to Python 3, we might as well start implementing Python 3 semantics for comparisons, which means raising an error when a comparison makes no sense.
comment:81 followup: 82 Changed 6 years ago by
To add to my last comment: we certainly should not make changes which will make the Python 3 porting even harder. I am afraid that your branch does that, since you rely more on the implementation of oldstyle Python 2 comparisons.
comment:82 followups: 83 84 Changed 6 years ago by
Replying to jdemeyer:
I am afraid that your branch does that, since you rely more on the implementation of oldstyle Python 2 comparisons.
Perhaps indeed, I'm not sure. Note that my version only returns NotImplemented
when comparing an Element
with something else. This by itself is correct (and perhaps desirable) in Python3 too, isn't it? The only thing that will change if what Python will do as a result.
I have no clear opinion regarding whether it is better to return NotImplemented
or to fail, and I trust you to know the details of Python3 comparison semantics better than me. The only thing I really care about is that nonsensical comparisons between Elements raise an exception.
However, I expect that raising an exception will break more existing code. Based on what I recall of the first version of my branch, the typical situation where falling back to the default Python2 comparison can help is with graphs or other combinatorial objects with labels that want to compare Elements with strings or with tuples. So, more than making Python3 porting harder, I think my version delays some of the issues with existing code to the actual porting.
I'm not going to try going back to a version that doesn't use NotImplemented
in the next few days. But if you have one that works well, I could try to find time to review it some time next week.
comment:83 Changed 6 years ago by
Replying to mmezzarobba:
Note that my version only returns
NotImplemented
when comparing anElement
with something else. This by itself is correct (and perhaps desirable) in Python3 too, isn't it?
Yes, return NotImplemented
is the right do to in Python 2. It is also the right thing to do in Python 3. So you would think that it's always right, but it's not.
The problem is that the implications of this return NotImplemented
are very different in Python 2 and Python 3. Python 2 will happily compare the objects anyway while Python 3 will raise an exception.
Given that we are planning to port to Python 3, it makes sense to adopt the Python 3 conventions already using Python 2. That is: raise an error for undefined comparisons.
comment:84 followup: 85 Changed 6 years ago by
Replying to mmezzarobba:
The only thing I really care about is that nonsensical comparisons between Elements raise an exception.
Then why don't you implement exactly that, without making changes to comparisons with nonElements? That sounds like a good idea to me.
comment:85 Changed 6 years ago by
Replying to jdemeyer:
Then why don't you implement exactly that, without making changes to comparisons with nonElements? That sounds like a good idea to me.
Because I don't think that's the best option, even if I care less about the other cases.
comment:86 Changed 5 years ago by
Description:  modified (diff) 

comment:87 Changed 5 years ago by
Keywords:  richcmp added; cmp removed 

comment:88 Changed 5 years ago by
Work issues:  → merge conflicts 

comment:89 Changed 5 years ago by
Work issues:  merge conflicts 

Please don't do that, it gives the totally wrong impression that this ticket needs work because there are merge conflicts.
comment:90 followup: 91 Changed 5 years ago by
Status:  needs_work → needs_info 

It would be nice if you could just add to that list instead of just removing my items. Merge conflicts make it hard to see what the proposed changes are doing without being able to click on the branch link, so I think that's an issue that needs work.
So, what are the proposed changes about? Last time I had a look at them I was not unhappy about them but you seemed to disagree. It might not be perfect but don't the proposed changes improve the situation at least?
comment:91 Changed 5 years ago by
It's too long ago. I don't remember.
Regardless, I feel that we should first focus on the "see also" tickets. In particular, #22349 looks like a major problem.
comment:92 Changed 5 years ago by
Work issues:  → merge conflicts & summarize what was controversial about this ticket 

comment:93 Changed 5 years ago by
Keywords:  python3 added 

comment:94 Changed 4 years ago by
Branch:  u/mmezzarobba/22029richcmp → u/jdemeyer/22029richcmp 

comment:95 Changed 4 years ago by
Authors:  Marc Mezzarobba → Marc Mezzarobba, Jeroen Demeyer 

Commit:  33e88b0c7b9d4ce464fd55da6725751b478f7662 → 8ef8033a7aaa6bbbc240624b3a0cfa29a559ce2f 
Status:  needs_info → needs_review 
Summary:  Element richcmp: never use id() → [WIP] Element richcmp: never use id() 
comment:96 Changed 4 years ago by
Commit:  8ef8033a7aaa6bbbc240624b3a0cfa29a559ce2f → 414ca3a344598e186ca67fbf9d8ee068dff3ce49 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
414ca3a  Element richcmp: never use id()

comment:98 Changed 4 years ago by
Description:  modified (diff) 

comment:99 Changed 4 years ago by
Not that many failed tests:
 sage t long warnlong 54.8 src/sage/schemes/elliptic_curves/height.py # 1 doctest failed sage t long warnlong 54.8 src/sage/graphs/generic_graph.py # 9 doctests failed sage t long warnlong 54.8 src/sage/symbolic/expression.pyx # 2 doctests failed sage t long warnlong 54.8 src/sage/homology/examples.py # 1 doctest failed sage t long warnlong 54.8 src/sage/combinat/tutorial.py # 4 doctests failed sage t long warnlong 54.8 src/sage/graphs/graph.py # 2 doctests failed sage t long warnlong 54.8 src/sage/homology/simplicial_complex.py # 12 doctests failed sage t long warnlong 54.8 src/sage/combinat/multiset_partition_into_sets_ordered.py # 3 doctests failed sage t long warnlong 54.8 src/sage/rings/valuation/valuation.py # 6 doctests failed sage t long warnlong 54.8 src/sage/combinat/finite_state_machine.py # 4 doctests failed sage t long warnlong 54.8 src/sage/graphs/digraph_generators.py # 4 doctests failed sage t long warnlong 54.8 src/sage/combinat/words/morphism.py # 5 doctests failed sage t long warnlong 54.8 src/sage/combinat/words/finite_word.py # 3 doctests failed sage t long warnlong 54.8 src/sage/combinat/words/word_generators.py # 1 doctest failed sage t long warnlong 54.8 src/sage/categories/algebra_functor.py # 1 doctest failed sage t long warnlong 54.8 src/sage/combinat/posets/lattices.py # 5 doctests failed sage t long warnlong 54.8 src/sage/structure/formal_sum.py # 3 doctests failed sage t long warnlong 54.8 src/sage/combinat/designs/incidence_structures.py # 15 doctests failed sage t long warnlong 54.8 src/sage/combinat/set_partition.py # 3 doctests failed sage t long warnlong 54.8 src/sage/misc/misc.py # 3 doctests failed sage t long warnlong 54.8 src/sage/structure/coerce.pyx # 1 doctest failed sage t long warnlong 54.8 src/sage/graphs/schnyder.py # 4 doctests failed 
Perhaps we should look at those failures and triage them into various tickets.
comment:100 Changed 4 years ago by
Description:  modified (diff) 

comment:101 Changed 4 years ago by
Work issues:  merge conflicts & summarize what was controversial about this ticket → dependencies 

comment:102 Changed 4 years ago by
Description:  modified (diff) 

comment:103 Changed 4 years ago by
Description:  modified (diff) 

comment:104 Changed 4 years ago by
Description:  modified (diff) 

comment:105 Changed 4 years ago by
Description:  modified (diff) 

comment:106 Changed 4 years ago by
Description:  modified (diff) 

comment:107 Changed 4 years ago by
Description:  modified (diff) 

comment:108 Changed 4 years ago by
Description:  modified (diff) 

comment:109 Changed 4 years ago by
I identified quite a few additional issues. I'm not saying that I found all issues (I skimmed semiquickly over the patchbot report) but surely the majority.
The most serious one is still the graph sorting, which counts for a substantial amount of doctest failures.
comment:110 Changed 4 years ago by
Description:  modified (diff) 

comment:111 Changed 4 years ago by
Description:  modified (diff) 

comment:112 Changed 4 years ago by
Commit:  414ca3a344598e186ca67fbf9d8ee068dff3ce49 → f680404ec637aa2ef550ec7d50f157d38431c46f 

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
9c92766  Add comparisons and hash for CuspFamily

bcc0a23  Do not sort terms in FormalSum

154b615  Sort key as sort_facets parameter of SimplicialComplex

3930c20  Do not sort in Subsets_s

d7ece06  Remove pointless comparison doctests in valuations

d0ad378  AbstractLanguage should work with uncomparable alphabet

7dda4ac  Use richcmp_item to compare Word_class

a354dc1  Do not sort points in IncidenceStructure

a9e0fe6  Merge commit 'bcc0a23bb4a32491f2145ff8412651a326b33a68'; commit '154b61536b55faca6f507c688881570fdc5812f8'; commit '3930c20b0d41c87f5b7ebc5c1ffd07bfcbb18832'; commit 'd7ece0605d390062cf625de4c54608cd05bfc8fa'; commit 'd0ad378c279b0628b84941ac1444c16b198cd7f4'; commit '7dda4ac6209a3b86ee0d042c2aa358d53f1d9f29'; commit 'a354dc129b0085acf88b4b1cbae1d5029218fa61' into t/22029/22029richcmp

f680404  Element richcmp: never use id()

comment:113 Changed 4 years ago by
Description:  modified (diff) 

comment:114 Changed 4 years ago by
Description:  modified (diff) 

comment:115 Changed 4 years ago by
Description:  modified (diff) 

comment:116 Changed 4 years ago by
Commit:  f680404ec637aa2ef550ec7d50f157d38431c46f → 2466c5f087c0235081caf05e5660ba16258d5098 

comment:117 Changed 4 years ago by
Commit:  2466c5f087c0235081caf05e5660ba16258d5098 → 34a2af2a32b48b9128b0641b53063d6c50da8134 

comment:118 Changed 4 years ago by
Description:  modified (diff) 

comment:119 Changed 4 years ago by
I looked at the patchbot failures again and I have now identified all issues.
comment:120 Changed 4 years ago by
Commit:  34a2af2a32b48b9128b0641b53063d6c50da8134 → f3aa7351801094a60ee2dd32003cbcfcd25e4e63 

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
30028e7  Element richcmp: never use id()

ea22e20  26936: fixing doctests with sortable alphabets

0cc0a61  26936: improved class documentation

db81e7d  26936: add example with non sortable alphabet

3efa9b1  _iterator_weight should not sort objects

a0f038f  py3: some fix in set_partition (sortable elements only)

94bc96a  make clear that equality needs a totally ordered set

4c0cd90  reword as requested

171f988  same for inequality

f3aa735  Merge commit 'db81e7dd2f141be4eed6cfcb87fc718ccae9e68c'; commit '3efa9b1ad4154e5f4516d437bfbc56ae47fa9d80'; commit '171f9885ff150378e69de4cdfeea2fb1d6b107ea' into t/22029/22029richcmp

comment:121 Changed 4 years ago by
Commit:  f3aa7351801094a60ee2dd32003cbcfcd25e4e63 → f54c5008dfcf5becd9de29b67f72daebe8fb95b5 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
f54c500  Merge commit 'db81e7dd2f141be4eed6cfcb87fc718ccae9e68c'; commit '3efa9b1ad4154e5f4516d437bfbc56ae47fa9d80'; commit '171f9885ff150378e69de4cdfeea2fb1d6b107ea' into t/22029/22029richcmp

comment:122 Changed 4 years ago by
Description:  modified (diff) 

comment:123 Changed 4 years ago by
Description:  modified (diff) 

comment:124 Changed 4 years ago by
Commit:  f54c5008dfcf5becd9de29b67f72daebe8fb95b5 → 66d2a8f776c811f02992a9bde3f41e85a3925e04 

Branch pushed to git repo; I updated commit sha1. New commits:
b4f67ca  Deprecate uniq()

6b000b9  Merge commit 'b4f67ca8620ffff6cf761abd44fe287a88c5bf13' into t/22029/22029richcmp

0240367  trac #27010: save some calls to .vertices()

3a409b1  Minor doctest fixes for Trac #22029

ab00c17  Avoid sorting vertices in relabel()

66d2a8f  Merge commit '0240367bef781fc219655b3bacd60ffd291eb595'; commit '3a409b1929c9a049772cb725aa579a0344dae0b4'; commit 'ab00c175178632d7dd1835bc618fa08bf5253aa8' into t/22029/22029richcmp

comment:125 Changed 4 years ago by
Description:  modified (diff) 

comment:126 Changed 4 years ago by
Commit:  66d2a8f776c811f02992a9bde3f41e85a3925e04 → 31240a827f25f09d659bd14bb18654c7f16939ac 

comment:127 Changed 4 years ago by
Description:  modified (diff) 

comment:128 Changed 4 years ago by
Commit:  31240a827f25f09d659bd14bb18654c7f16939ac → d5d7d70ea906fb736538bd2dbc62ddf03a5911a0 

comment:129 Changed 4 years ago by
Commit:  d5d7d70ea906fb736538bd2dbc62ddf03a5911a0 → 4c75779bdb1a75db1856fafb265523aae60569d4 

Branch pushed to git repo; I updated commit sha1. New commits:
a88dc2c  Make relabel() of DynkinDiagram more compatible with GenericGraph

5ec5eb0  Avoid calling vertices() in graph_isom_equivalent_non_edge_labeled_graph()

4c75779  Merge commit '5ec5eb0d4dc1a04dd51e216949841933256f7174' into t/22029/22029richcmp

comment:130 Changed 4 years ago by
Description:  modified (diff) 

comment:131 followup: 132 Changed 4 years ago by
I consider this ticket to be essentially finished. We need to wait a bit until all dependencies have been merged and a new Pynac release has been made.
comment:132 Changed 4 years ago by
Replying to jdemeyer:
I consider this ticket to be essentially finished. We need to wait a bit until all dependencies have been merged and a new Pynac release has been made.
This is great! Thank you for working on it.
comment:133 Changed 4 years ago by
Commit:  4c75779bdb1a75db1856fafb265523aae60569d4 → f3890d675857384620b5a65e18ee0b280efef6bc 

comment:134 Changed 4 years ago by
Commit:  f3890d675857384620b5a65e18ee0b280efef6bc → b5f1fd618b40c4f5f7bc9bbd56de77f344d7ec5b 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
b5f1fd6  Element richcmp: never use id()

comment:135 Changed 4 years ago by
Commit:  b5f1fd618b40c4f5f7bc9bbd56de77f344d7ec5b → 815ceab37e0b72270040a790b1a79b43d7d5c455 

comment:136 Changed 4 years ago by
Commit:  815ceab37e0b72270040a790b1a79b43d7d5c455 → e09ddd2dbfd91001567c43693a1e90a7ee6bbaed 

comment:137 Changed 4 years ago by
Description:  modified (diff) 

comment:138 Changed 4 years ago by
Commit:  e09ddd2dbfd91001567c43693a1e90a7ee6bbaed → 21987572be7170cc85cd4b701b08bf565fe27eb7 

comment:139 Changed 4 years ago by
Description:  modified (diff) 

Milestone:  sage7.5 → sage8.7 
comment:140 Changed 4 years ago by
Commit:  21987572be7170cc85cd4b701b08bf565fe27eb7 → a0832c075ffdca8ef8f88c009098cb96ee811b06 

comment:141 Changed 4 years ago by
Commit:  a0832c075ffdca8ef8f88c009098cb96ee811b06 → 20b647725d977fa5be5bfe712b21987dfc8fcecc 

comment:142 Changed 4 years ago by
Commit:  20b647725d977fa5be5bfe712b21987dfc8fcecc → ef01c332d8a7dd4ddab0c796888d065c0abefb4f 

comment:143 Changed 4 years ago by
Dependencies:  → #27221 

Description:  modified (diff) 
Summary:  [WIP] Element richcmp: never use id() → Element richcmp: never use id() 
Work issues:  dependencies 
comment:145 Changed 4 years ago by
Dependencies:  #27221 → #27241 

Description:  modified (diff) 
comment:146 Changed 4 years ago by
Branch:  u/jdemeyer/22029richcmp → u/mmezzarobba/22029richcmp 

Commit:  ef01c332d8a7dd4ddab0c796888d065c0abefb4f → e311ab134f9d0c16dbbb800d35ba2c4325b4beb2 
comment:147 followup: 149 Changed 4 years ago by
As for the actual content: I'm happy with this version, and I'm ready to give it positive review once the patchbot comes back green with the new pynac release. I'm still not 100% convinced that the way you are handling nonElements and coercion failures is optimal, but even if not, it shouldn't be too hard to change later if this turns out to be an issue.
comment:148 Changed 4 years ago by
Reviewers:  Julian Rüth → Julian Rüth, Marc Mezzarobba 

Status:  needs_review → positive_review 
The tests pass on my box.
comment:149 followup: 150 Changed 4 years ago by
Replying to mmezzarobba:
I'm still not 100% convinced that the way you are handling nonElements and coercion failures is optimal
What do you mean exactly? I'm open to suggestions for improvement.
What I want is that comparisons involving the coercion model behave just like comparisons in Python 3 should behave. This means: if the objects cannot be compared, raise an error instead of returning some arbitrary value.
comment:150 followup: 151 Changed 4 years ago by
Replying to jdemeyer:
Replying to mmezzarobba:
I'm still not 100% convinced that the way you are handling nonElements and coercion failures is optimal
What do you mean exactly? I'm open to suggestions for improvement.
We had this discussion two years ago when I started this ticket, and frankly I don't remember all the details. But I think it revolved around the following points:
 Regarding coercion failures, it might be better to raise an error (however inconvenient that would be!)
 instead of returning
False
when twoElement
s with no common parent are compared using==
,  and, perhaps more importantly, instead of returning
True
when they are compared with!=
—the additional point to consider in this case being that returningTrue
would be “wrong” if we haven't proved that the elements are different, while many boolean predicates in Sage returnFalse
when they can not decide.
 instead of returning
 Regarding non
Element
s: The coercion framework treats some specific types essentially like
Element
. We need to be consistent with that, but still allow users to define new types that implement their own comparison logic with Sage elements. richcmp()
currently does that by callingtp_richcompare()
on the nonElement
, instead of returningNotImplemented
. I'm not 100% comfortable with this choice. I assume that the goal to get the same behavior under Python 2 and 3, but I'm a bit confused here, because (unless I misunderstood you) you argued against that choice two years ago.
 The coercion framework treats some specific types essentially like
comment:151 Changed 4 years ago by
Replying to mmezzarobba:
 Regarding coercion failures, it might be better to raise an error (however inconvenient that would be!)
 instead of returning
False
when twoElement
s with no common parent are compared using==
I would argue that, when there is no common parent, elements clearly cannot be equal. This is also consistent with Python 3 where uncomparable objects can still be compared by ==
and !=
:
Python 3.6.3 (default, Mar 13 2018, 19:00:30) [GCC 6.4.0] on linux Type "help", "copyright", "credits" or "license" for more information. >>> 1 == "x" False
 and, perhaps more importantly, instead of returning
True
when they are compared with!=
—the additional point to consider in this case being that returningTrue
would be “wrong” if we haven't proved that the elements are different
Again, if those elements don't coerce to a common parent, they cannot be equal.
 Regarding non
Element
s:
 The coercion framework treats some specific types essentially like
Element
. We need to be consistent with that
As far as I know, we are consistent.
but still allow users to define new types that implement their own comparison logic with Sage elements.
richcmp()
currently does that by callingtp_richcompare()
on the nonElement
, instead of returningNotImplemented
.
Exactly.
I'm not 100% comfortable with this choice. I assume that the goal to get the same behavior under Python 2 and 3
Exactly. Returning NotImplemented
causes different behavior on Python 2 and Python 3.
I'm a bit confused here, because (unless I misunderstood you) you argued against that choice two years ago.
I don't remember precisely. Anyway, I changed my mind and I'm convinced now that raising an error is the right thing to do.
comment:152 Changed 4 years ago by
Branch:  u/mmezzarobba/22029richcmp → e311ab134f9d0c16dbbb800d35ba2c4325b4beb2 

Resolution:  → fixed 
Status:  positive_review → closed 
Do you plan to work on this? If yes, fill in your name as reviewer. If no, let me know.