Opened 3 years ago

Closed 3 years ago

#21995 closed defect (fixed)

Fix comparison operators of PoorManMap

Reported by: saraedum Owned by:
Priority: minor Milestone: sage-7.5
Component: categories Keywords:
Cc: Merged in:
Authors: Julian Rüth Reviewers: David Roe
Report Upstream: N/A Work issues:
Branch: 7776c16 (Commits) Commit: 7776c1644f1fef5c35a8cf6dd76dd45711692e51
Dependencies: Stopgaps:

Description (last modified by saraedum)

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_methods in PoorManMap?, see #21894.

Change History (28)

comment:1 Changed 3 years ago by saraedum

  • Branch set to u/saraedum/fix_comparison_operators_of_poormanmap

comment:2 Changed 3 years ago by saraedum

  • Commit set to 0d0f55d089dd299bfd36b6b441f142e2fbdd8fd8
  • Description modified (diff)

New commits:

0d0f55dImplement _richcmp_ and __hash__ for PoorManMap

comment:3 Changed 3 years ago by saraedum

  • 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 3 years ago by saraedum

  • 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 3 years ago by git

  • Commit changed from 0d0f55d089dd299bfd36b6b441f142e2fbdd8fd8 to 3f20aaa44c15ff93b816fe787742ad60d390f37c

Branch pushed to git repo; I updated commit sha1. New commits:

40a6d5ba PoorManMap is unequal to anything else
3f20aaaMake PoorManComposeMap correctly inherit from PoorManMap

comment:6 Changed 3 years ago by saraedum

  • Status changed from needs_work to needs_review

comment:7 Changed 3 years ago by saraedum

  • 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 3 years ago by git

  • Commit changed from 3f20aaa44c15ff93b816fe787742ad60d390f37c to 554bae8ecb9e67741de85f6b71aca09a173a1519

Branch pushed to git repo; I updated commit sha1. New commits:

554bae8Removed PoorManComposeMap

comment:9 Changed 3 years ago by saraedum

  • Status changed from needs_work to needs_review

comment:10 Changed 3 years ago by saraedum

  • Description modified (diff)

comment:11 Changed 3 years ago by saraedum

  • Description modified (diff)

comment:12 Changed 3 years ago by roed

Looks good to me.

comment:13 Changed 3 years ago by roed

  • Reviewers set to David Roe
  • Status changed from needs_review to positive_review

comment:14 Changed 3 years ago by git

  • 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:

736fb42Change license of poor_man_map to GPLv2+

comment:15 Changed 3 years ago by roed

  • Status changed from needs_review to positive_review

comment:16 follow-up: Changed 3 years ago by caruso

  • 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.)

Last edited 3 years ago by caruso (previous) (diff)

comment:17 Changed 3 years ago by caruso

  • Status changed from needs_info to needs_work

comment:18 Changed 3 years ago by tscrim

Mostly to cc myself, but there should not be periods at the ends of each of the INPUT:s.

comment:19 Changed 3 years ago by saraedum

comment:20 Changed 3 years ago by tscrim

*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 3 years ago by git

  • Commit changed from 736fb42902d2436dabbd0d6dbfed9154af151804 to db76a47e3c0d47e17f59ca97555a92bd0b593ac9

Branch pushed to git repo; I updated commit sha1. New commits:

db76a47remove incorrect periods from docstring

comment:22 in reply to: ↑ 16 Changed 3 years ago by saraedum

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 3 years ago by git

  • Commit changed from db76a47e3c0d47e17f59ca97555a92bd0b593ac9 to 66952f42878728c4132b2dba3aba045569b147b0

Branch pushed to git repo; I updated commit sha1. New commits:

282b237remove tab
66952f4Check compatibility of morphisms when composing

comment:24 Changed 3 years ago by saraedum

  • Status changed from needs_work to needs_review

comment:25 Changed 3 years ago by caruso

Looks fine to me.

Just a small remark: in the doctest of __call__, it's better to use g*f than f*g.

Last edited 3 years ago by caruso (previous) (diff)

comment:26 Changed 3 years ago by git

  • Commit changed from 66952f42878728c4132b2dba3aba045569b147b0 to 7776c1644f1fef5c35a8cf6dd76dd45711692e51

Branch pushed to git repo; I updated commit sha1. New commits:

7776c16Fix doctest

comment:27 Changed 3 years ago by caruso

  • Status changed from needs_review to positive_review

Ok, thanks. That's nice.

comment:28 Changed 3 years ago by vbraun

  • Branch changed from u/saraedum/fix_comparison_operators_of_poormanmap to 7776c1644f1fef5c35a8cf6dd76dd45711692e51
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.