Opened 3 years ago

Closed 2 years ago

#19820 closed defect (fixed)

Improve __eq__ for triangular module morphisms

Reported by: tscrim Owned by: tscrim
Priority: major Milestone: sage-7.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) Commit: 5b1b761250b8c384d0cbd53695b73ad5f15f2aa4
Dependencies: Stopgaps:

Description (last modified by saraedum)

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

  • Branch set to public/morphisms/triangular_module_morphism_eq-19820
  • Commit set to 16c0bfa0f5855ce030239b14a9b1b806b5447cd6
  • Status changed from new to needs_review

New commits:

16c0bfaBetter __eq__ for triangular module morphisms.

comment:2 Changed 3 years ago by chapoton

  • Status changed from needs_review to needs_work

some failing doctests, due to use of cmp instead of key

comment:3 Changed 3 years ago by saraedum

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.

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

comment:4 Changed 3 years ago by git

  • Commit changed from 16c0bfa0f5855ce030239b14a9b1b806b5447cd6 to 6f6f0d337cd53ccb0886e19d37e2731f625e4426

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

d4625f4Merge branch 'develop' into t/19820/public/morphisms/triangular_module_morphism_eq-19820
f3f468dImplement __eq__ for PointwiseInverse properly
c6093d5Also implement __ne__ as it is not magically added from somewhere else
55ab282Implement _richcmp_ for ModuleMorphismByLinearity
031d86cImplement TriangularModuleMorphism._richcmp_
b7c99e7Implement TriangularModuleMorphismByLinearity._richcmp_
edef6c3Implement ModuleMorphismFromMatrix._richcmp_
0a9a17fImplement DiagonalModuleMorphism._richcmp_
6f6f0d3Change __eq__ docstring wording

comment:5 Changed 3 years ago by saraedum

  • Authors changed from Travis Scrimshaw to Travis Scrimshaw, Julian Rüth
  • 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 3 years ago by git

  • Commit changed from 6f6f0d337cd53ccb0886e19d37e2731f625e4426 to 256f4879fac574b963f4decb8e12a72c833ef844

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

256f487expanded tabs

comment:7 Changed 3 years ago by nthiery

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

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

  • Commit changed from 256f4879fac574b963f4decb8e12a72c833ef844 to af447b267f33d568a1dfcf9a60c7aa2e07f30dd4

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

af447b2Expand cmp

comment:10 Changed 3 years ago by saraedum

The patchbot still complains about a few cmps. These are, however, not actual invocations of the cmp function of python but just a keyword argument that is called cmp.


New commits:

af447b2Expand cmp

New commits:

af447b2Expand cmp

comment:11 Changed 3 years ago by chapoton

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

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

comment:12 Changed 3 years ago by saraedum

  • Description modified (diff)

comment:13 Changed 3 years ago by saraedum

It would be great if somebody who understands what these classes are supposed to do could have a look at my changes. This is a dependency of #21996. But sadly, the people who care about #21996 are not exactly sure what these classes here are doing, so we can not review this ourselves. Thanks!

comment:14 Changed 3 years ago by chapoton

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

  • Commit changed from af447b267f33d568a1dfcf9a60c7aa2e07f30dd4 to d9a4a75c1ad295650cd7f5bd172ab37a26d55bf4

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

d9a4a75Do not use magic constants in richcmp

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

Thanks, I did not know that there was op_NE.

comment:17 Changed 2 years ago by git

  • Commit changed from d9a4a75c1ad295650cd7f5bd172ab37a26d55bf4 to bfcdfe5cddc243af2679ef1497f84edafcb2fb2d

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

60a1ab4Merge branch 'public/morphisms/triangular_module_morphism_eq-19820' of trac.sagemath.org:sage into public/morphisms/triangular_module_morphism_eq-19820
bfcdfe5Some little bits of cleanup.

comment:18 Changed 2 years ago by tscrim

  • Keywords days85 added
  • Milestone changed from sage-7.0 to sage-7.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 2 years ago by jdemeyer

  • 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 2 years ago by jdemeyer

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 2 years ago by jdemeyer

Just remove this line instead of commenting it:

#from sage.misc.cachefunc import cached_method, cached_function
Last edited 2 years ago by jdemeyer (previous) (diff)

comment:22 in reply to: ↑ 16 Changed 2 years ago by jdemeyer

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

  • Commit changed from bfcdfe5cddc243af2679ef1497f84edafcb2fb2d to 5b1b761250b8c384d0cbd53695b73ad5f15f2aa4

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

5b1b761Fixing some tidbits of ModuleMorphism.

comment:24 Changed 2 years ago by tscrim

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

  • Status changed from needs_review to positive_review

comment:26 Changed 2 years ago by vbraun

  • Branch changed from public/morphisms/triangular_module_morphism_eq-19820 to 5b1b761250b8c384d0cbd53695b73ad5f15f2aa4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.