Opened 5 years ago
Closed 5 years ago
#21995 closed defect (fixed)
Fix comparison operators of PoorManMap
Reported by:  saraedum  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 5 years ago by
 Branch set to u/saraedum/fix_comparison_operators_of_poormanmap
comment:2 Changed 5 years ago by
 Commit set to 0d0f55d089dd299bfd36b6b441f142e2fbdd8fd8
 Description modified (diff)
comment:3 Changed 5 years ago by
 Status changed from new to needs_review
I still have to wait for my sage to build to check that this actually works.
comment:4 Changed 5 years ago by
 Status changed from needs_review to needs_work
I just realized that PoorManMap? is inherited from PoorManMapCompose?. Got to fix that one as well.
comment:5 Changed 5 years ago by
 Commit changed from 0d0f55d089dd299bfd36b6b441f142e2fbdd8fd8 to 3f20aaa44c15ff93b816fe787742ad60d390f37c
comment:6 Changed 5 years ago by
 Status changed from needs_work to needs_review
comment:7 Changed 5 years ago by
 Status changed from needs_review to needs_work
Xavier Caruso proposed to simplify this by integrating the compose map logic into PoorManMap
.
comment:8 Changed 5 years ago by
 Commit changed from 3f20aaa44c15ff93b816fe787742ad60d390f37c to 554bae8ecb9e67741de85f6b71aca09a173a1519
Branch pushed to git repo; I updated commit sha1. New commits:
554bae8  Removed PoorManComposeMap

comment:9 Changed 5 years ago by
 Status changed from needs_work to needs_review
comment:10 Changed 5 years ago by
 Description modified (diff)
comment:11 Changed 5 years ago by
 Description modified (diff)
comment:12 Changed 5 years ago by
Looks good to me.
comment:13 Changed 5 years ago by
 Reviewers set to David Roe
 Status changed from needs_review to positive_review
comment:14 Changed 5 years ago by
 Commit changed from 554bae8ecb9e67741de85f6b71aca09a173a1519 to 736fb42902d2436dabbd0d6dbfed9154af151804
 Status changed from positive_review to 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 5 years ago by
 Status changed from needs_review to positive_review
comment:16 followup: ↓ 22 Changed 5 years ago by
 Status changed from positive_review to 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 5 years ago by
 Status changed from needs_info to needs_work
comment:18 Changed 5 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 5 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 5 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 5 years ago by
 Commit changed from 736fb42902d2436dabbd0d6dbfed9154af151804 to db76a47e3c0d47e17f59ca97555a92bd0b593ac9
Branch pushed to git repo; I updated commit sha1. New commits:
db76a47  remove incorrect periods from docstring

comment:22 in reply to: ↑ 16 Changed 5 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 5 years ago by
 Commit changed from db76a47e3c0d47e17f59ca97555a92bd0b593ac9 to 66952f42878728c4132b2dba3aba045569b147b0
comment:24 Changed 5 years ago by
 Status changed from needs_work to needs_review
comment:25 Changed 5 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 5 years ago by
 Commit changed from 66952f42878728c4132b2dba3aba045569b147b0 to 7776c1644f1fef5c35a8cf6dd76dd45711692e51
Branch pushed to git repo; I updated commit sha1. New commits:
7776c16  Fix doctest

comment:27 Changed 5 years ago by
 Status changed from needs_review to positive_review
Ok, thanks. That's nice.
comment:28 Changed 5 years ago by
 Branch changed from u/saraedum/fix_comparison_operators_of_poormanmap to 7776c1644f1fef5c35a8cf6dd76dd45711692e51
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Implement _richcmp_ and __hash__ for PoorManMap