Opened 6 years ago
Closed 6 years ago
#21995 closed defect (fixed)
Fix comparison operators of PoorManMap
Reported by:  Julian Rüth  Owned by:  

Priority:  minor  Milestone:  sage7.5 
Component:  categories  Keywords:  
Cc:  Merged in:  
Authors:  Julian Rüth  Reviewers:  David Roe 
Report Upstream:  N/A  Work issues:  
Branch:  7776c16 (Commits, GitHub, GitLab)  Commit:  7776c1644f1fef5c35a8cf6dd76dd45711692e51 
Dependencies:  Stopgaps: 
Description (last modified by )
PoorManMap? currently only implements __eq__
but not __hash__
nor __ne__
.
This ticket implements __eq__
, __ne__
, and __hash__
and makes these not rely on comparison of __dict__
which does not work anymore when introducing @cached_method
s in PoorManMap?, see #21894.
Change History (28)
comment:1 Changed 6 years ago by
Branch:  → u/saraedum/fix_comparison_operators_of_poormanmap 

comment:2 Changed 6 years ago by
Commit:  → 0d0f55d089dd299bfd36b6b441f142e2fbdd8fd8 

Description:  modified (diff) 
comment:3 Changed 6 years ago by
Status:  new → needs_review 

I still have to wait for my sage to build to check that this actually works.
comment:4 Changed 6 years ago by
Status:  needs_review → needs_work 

I just realized that PoorManMap? is inherited from PoorManMapCompose?. Got to fix that one as well.
comment:5 Changed 6 years ago by
Commit:  0d0f55d089dd299bfd36b6b441f142e2fbdd8fd8 → 3f20aaa44c15ff93b816fe787742ad60d390f37c 

comment:6 Changed 6 years ago by
Status:  needs_work → needs_review 

comment:7 Changed 6 years ago by
Status:  needs_review → needs_work 

Xavier Caruso proposed to simplify this by integrating the compose map logic into PoorManMap
.
comment:8 Changed 6 years ago by
Commit:  3f20aaa44c15ff93b816fe787742ad60d390f37c → 554bae8ecb9e67741de85f6b71aca09a173a1519 

Branch pushed to git repo; I updated commit sha1. New commits:
554bae8  Removed PoorManComposeMap

comment:9 Changed 6 years ago by
Status:  needs_work → needs_review 

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

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

comment:13 Changed 6 years ago by
Reviewers:  → David Roe 

Status:  needs_review → positive_review 
comment:14 Changed 6 years ago by
Commit:  554bae8ecb9e67741de85f6b71aca09a173a1519 → 736fb42902d2436dabbd0d6dbfed9154af151804 

Status:  positive_review → needs_review 
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
736fb42  Change license of poor_man_map to GPLv2+

comment:15 Changed 6 years ago by
Status:  needs_review → positive_review 

comment:16 followup: 22 Changed 6 years ago by
Status:  positive_review → needs_info 

(Sorry, I got troubles with my Internet connexion yesterday.)
I've just a remark: shouldn't we check that self.domain() == other.codomain() (if the are both defined) in the composition? Besides, the example you give in the doctest is a kind of weird since the composite f*g does not take {2,3,4} to itself. (I suppose you wanted to write g*f... this proves that an additional checking is not unnecessary.)
comment:17 Changed 6 years ago by
Status:  needs_info → needs_work 

comment:18 Changed 6 years ago by
Mostly to cc myself, but there should not be periods at the ends of each of the INPUT:
s.
comment:19 Changed 6 years ago by
tscrim: I agree. But there's actually an example for that here http://doc.sagemath.org/html/en/developer/coding_basics.html#thedocstringofafunctioncontent
comment:20 Changed 6 years ago by
*grumbles as bangs head into desk* That explains the uptake in (grammatically incorrect) periods. Looking further, there are a couple of other things to fix there too. I will fix all of that on a separate ticket.
comment:21 Changed 6 years ago by
Commit:  736fb42902d2436dabbd0d6dbfed9154af151804 → db76a47e3c0d47e17f59ca97555a92bd0b593ac9 

Branch pushed to git repo; I updated commit sha1. New commits:
db76a47  remove incorrect periods from docstring

comment:22 Changed 6 years ago by
Replying to caruso:
I've just a remark: shouldn't we check that self.domain() == other.codomain() (if the are both defined) in the composition? Besides, the example you give in the doctest is a kind of weird since the composite f*g does not take {2,3,4} to itself. (I suppose you wanted to write g*f... this proves that an additional checking is not unnecessary.)
Oops. I fixed that doctest. And I also added some checks when the domains and codomains are parents. But if they are just iterables, I am not sure what to do? Checking for equality seems to restrictive: for f*g
, the codomain of g
should be contained in the domain of f
but they do not have to be equal I think. In any case, comparing them (whether for equality or containment) is quite costly, i.e., O(cardinality) at best. And I would need to special case if the (co)domain is an iterable because then it could well be infinite.
What do you think?
comment:23 Changed 6 years ago by
Commit:  db76a47e3c0d47e17f59ca97555a92bd0b593ac9 → 66952f42878728c4132b2dba3aba045569b147b0 

comment:24 Changed 6 years ago by
Status:  needs_work → needs_review 

comment:25 Changed 6 years ago by
Looks fine to me.
Just a small remark: in the doctest of __call__
, it's better to use g*f than f*g.
comment:26 Changed 6 years ago by
Commit:  66952f42878728c4132b2dba3aba045569b147b0 → 7776c1644f1fef5c35a8cf6dd76dd45711692e51 

Branch pushed to git repo; I updated commit sha1. New commits:
7776c16  Fix doctest

comment:28 Changed 6 years ago by
Branch:  u/saraedum/fix_comparison_operators_of_poormanmap → 7776c1644f1fef5c35a8cf6dd76dd45711692e51 

Resolution:  → fixed 
Status:  positive_review → closed 
New commits:
Implement _richcmp_ and __hash__ for PoorManMap