Opened 6 years ago
Closed 5 years ago
#19820 closed defect (fixed)
Improve __eq__ for triangular module morphisms
Reported by:  tscrim  Owned by:  tscrim 

Priority:  major  Milestone:  sage7.6 
Component:  categories  Keywords:  triangular module morphism, days85 
Cc:  darij, nthiery, SimonKing  Merged in:  
Authors:  Travis Scrimshaw, Julian Rüth  Reviewers:  Julian Rüth, Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  5b1b761 (Commits, GitHub, GitLab)  Commit:  5b1b761250b8c384d0cbd53695b73ad5f15f2aa4 
Dependencies:  Stopgaps: 
Description (last modified by )
Right now, a comparison by the underlying __dict__
is done, which causes problems when trying to define a cached method on a morphism. We need to implement a better __eq__
for each subclass with more controllable behavior. See the discussion on #19609.
See also #21894 which collects similarly broken implementations of __eq__
.
Change History (26)
comment:1 Changed 6 years ago by
 Branch set to public/morphisms/triangular_module_morphism_eq19820
 Commit set to 16c0bfa0f5855ce030239b14a9b1b806b5447cd6
 Status changed from new to needs_review
comment:2 Changed 5 years ago by
 Status changed from needs_review to needs_work
some failing doctests, due to use of cmp instead of key
comment:3 Changed 5 years ago by
Why is there no __ne__
implementation? I do not really see where such an implementation should come from, i.e., it just checks inequality on generators I believe. In fact (without claiming that this has any mathematical content)
sage: f = X.module_morphism(on_basis=X.monomial, zero=X.monomial(0), category=Sets()) sage: sage: g = X.module_morphism(on_basis=X.monomial, zero=X.zero(), category=Sets()) sage: sage: f == g False sage: f != g False
So, I guess we should override the _richcmp_
implementation instead.
comment:4 Changed 5 years ago by
 Commit changed from 16c0bfa0f5855ce030239b14a9b1b806b5447cd6 to 6f6f0d337cd53ccb0886e19d37e2731f625e4426
Branch pushed to git repo; I updated commit sha1. New commits:
d4625f4  Merge branch 'develop' into t/19820/public/morphisms/triangular_module_morphism_eq19820

f3f468d  Implement __eq__ for PointwiseInverse properly

c6093d5  Also implement __ne__ as it is not magically added from somewhere else

55ab282  Implement _richcmp_ for ModuleMorphismByLinearity

031d86c  Implement TriangularModuleMorphism._richcmp_

b7c99e7  Implement TriangularModuleMorphismByLinearity._richcmp_

edef6c3  Implement ModuleMorphismFromMatrix._richcmp_

0a9a17f  Implement DiagonalModuleMorphism._richcmp_

6f6f0d3  Change __eq__ docstring wording

comment:5 Changed 5 years ago by
 Status changed from needs_work to needs_review
Travis: Could you have a look at my changes? I implemented _richcmp_
, to provide an implementation of !=
. I also, removed the comparison on _cmp
since I did not understand where this property would come from. Finally, I removed one call to the superclass, since self._on_basis == other._on_basis
causes an infinite recursion when _on_basis
is a normal method since comparing it triggers comparison of the im_self
attribute…
To be honest, I do not really understand what much if this code is about, so I might have broken something.
comment:6 Changed 5 years ago by
 Commit changed from 6f6f0d337cd53ccb0886e19d37e2731f625e4426 to 256f4879fac574b963f4decb8e12a72c833ef844
Branch pushed to git repo; I updated commit sha1. New commits:
256f487  expanded tabs

comment:7 Changed 5 years ago by
Just my 2 cents: IIRC, the main motivation so far for the implementation of __eq__
was to please pickling tests. We can't hope to have ==
on triangular module morphisms to be mathematical equality in general (this would require to have mathematical equality on arbitrary Python functions). If there is an easy way to get "syntactical equality" (e.g. when a morphism is built twice from the same function), that's a nice improvement.
Cheers,
comment:8 Changed 5 years ago by
nthiery: Sure. As we discussed at some point, mathematical equality is probably not what we should aim for in the first place: ==
should detect whether two objects are indistinguishable (in a reasonable sense,) everything else does not play well with dictionaries, hashes, … in the long run.
Anyway, if we have ==
we should at least have a !=
.
comment:9 Changed 5 years ago by
 Commit changed from 256f4879fac574b963f4decb8e12a72c833ef844 to af447b267f33d568a1dfcf9a60c7aa2e07f30dd4
Branch pushed to git repo; I updated commit sha1. New commits:
af447b2  Expand cmp

comment:10 Changed 5 years ago by
comment:11 Changed 5 years ago by
warning: this is going to conflict very heavily with one closed python3 ticket, which already did the same job
EDIT: or maybe not, I confused two morphism files
comment:12 Changed 5 years ago by
 Description modified (diff)
comment:13 Changed 5 years ago by
comment:14 Changed 5 years ago by
I have been told not to use magic numbers "op == 2" but rather "op == Py_NE" or "op == op_NE", imported from somwhere.
comment:15 Changed 5 years ago by
 Commit changed from af447b267f33d568a1dfcf9a60c7aa2e07f30dd4 to d9a4a75c1ad295650cd7f5bd172ab37a26d55bf4
Branch pushed to git repo; I updated commit sha1. New commits:
d9a4a75  Do not use magic constants in richcmp

comment:16 followup: ↓ 22 Changed 5 years ago by
Thanks, I did not know that there was op_NE
.
comment:17 Changed 5 years ago by
 Commit changed from d9a4a75c1ad295650cd7f5bd172ab37a26d55bf4 to bfcdfe5cddc243af2679ef1497f84edafcb2fb2d
comment:18 Changed 5 years ago by
 Keywords days85 added
 Milestone changed from sage7.0 to sage7.6
 Reviewers set to Julian Rüth, Travis Scrimshaw
I made it return NotImplemented
instead of raising an error because that is the more standard way of doing things. Specifically it allows the other object to try its comparison operations. If my changes are good, then positive review.
comment:19 Changed 5 years ago by
 Status changed from needs_review to needs_work
_richcmp_()
will only get called for equal parents. So this check is not needed: self.parent() == other.parent()
(just pointing this point, I don't actually care about this ticket)
comment:20 Changed 5 years ago by
In case that this parent only has one element class (which is often the case), also the check
self.__class__ is other.__class__
can be skipped.
comment:21 Changed 5 years ago by
Just remove this line instead of commenting it:
#from sage.misc.cachefunc import cached_method, cached_function
comment:22 in reply to: ↑ 16 Changed 5 years ago by
Replying to saraedum:
Thanks, I did not know that there was
op_NE
.
It was added pretty recently to Sage. It is there to allow pure Python code to use/implement rich comparisons the same way as Cython.
comment:23 Changed 5 years ago by
 Commit changed from bfcdfe5cddc243af2679ef1497f84edafcb2fb2d to 5b1b761250b8c384d0cbd53695b73ad5f15f2aa4
Branch pushed to git repo; I updated commit sha1. New commits:
5b1b761  Fixing some tidbits of ModuleMorphism.

comment:24 Changed 5 years ago by
 Status changed from needs_work to needs_review
Essentially all of these could be created from the same parent (Hom(V,W)
) as the classes represent certain special properties of the morphism. So we have to check the class. However, I've removed the commented line and the parent comparisons.
comment:25 Changed 5 years ago by
 Status changed from needs_review to positive_review
comment:26 Changed 5 years ago by
 Branch changed from public/morphisms/triangular_module_morphism_eq19820 to 5b1b761250b8c384d0cbd53695b73ad5f15f2aa4
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Better __eq__ for triangular module morphisms.