Opened 4 years ago

Closed 4 years ago

#24298 closed enhancement (fixed)

Improve Morphism.is_identity()

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.1
Component: categories Keywords:
Cc: tscrim Merged in:
Authors: Jeroen Demeyer Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 3479550 (Commits, GitHub, GitLab) Commit: 3479550c0e9d32362144072855fd116f41dc4220
Dependencies: #24281 Stopgaps:

Status badges

Description (last modified by jdemeyer)

Just make phi.is_identity() an alias of phi == phi.parent().identity()

Change History (12)

comment:1 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 4 years ago by jdemeyer

  • Branch set to u/jdemeyer/ticket/24298

comment:3 Changed 4 years ago by jdemeyer

  • Commit set to 29ad327949cf4067c46f120a33e49e17968f3cd9
  • Status changed from new to needs_review

New commits:

25dc2fdFix comparison of morphisms
29ad327Improve Morphism.is_identity()

comment:4 follow-up: Changed 4 years ago by tscrim

  • Reviewers set to Travis Scrimshaw

What do you think about just having:

def is_identity(self):
   return self._richcmp_(self._parent.identity(), Py_EQ)

and letting the parent handle the verification of being an endomorphism ring? IMO, this is more future-proof if we end up having a subclass Endset of Homset, where we could avoid the check. We might also want to cache the result of identity so we don't have to recreate a (simple) identity map on every call.

Otherwise LGTM.

comment:5 in reply to: ↑ 4 Changed 4 years ago by jdemeyer

Replying to tscrim:

What do you think about just having:

def is_identity(self):
   return self._richcmp_(self._parent.identity(), Py_EQ)

That would be slightly less efficient because of the extra level of indirection. But you are right in principle.

comment:6 Changed 4 years ago by git

  • Commit changed from 29ad327949cf4067c46f120a33e49e17968f3cd9 to 3479550c0e9d32362144072855fd116f41dc4220

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

3479550Improve Morphism.is_identity()

comment:7 Changed 4 years ago by jdemeyer

Like this?

comment:8 Changed 4 years ago by tscrim

There would be indirection until we have identity being a @cached_method (except for the first call of course). Yes, I do like that better (I forgot we do not want to propagate the error). Although it will always be slower in returning false because it needs to throw and catch the error. I guess there is a question of how much do we expect that users call this on non-endomorphism homsets? We should just override is_endomorphism_set to return True for the endset category, which is now #24304.

comment:9 follow-up: Changed 4 years ago by tscrim

What I am asking if you are okay with a speed penalty here for when a user doesn't realize that they are trying to check if it is the identity morphism over something that is not an endomorphism ring.

I do plan on going further with #24304 and moving stuff from #24277 up to the category level (but at least for tonight I do not want to watch morphism.pyx and all the things that depend on it recompile ^^;; ).

comment:10 in reply to: ↑ 9 Changed 4 years ago by jdemeyer

Replying to tscrim:

What I am asking if you are okay with a speed penalty here for when a user doesn't realize that they are trying to check if it is the identity morphism over something that is not an endomorphism ring.

I think so. Calling is_identity() is pretty meaningless on a non-Endset so I hope that it doesn't happen often in serious code.

comment:11 Changed 4 years ago by tscrim

  • Status changed from needs_review to positive_review

Then positive review. Thank you.

comment:12 Changed 4 years ago by vbraun

  • Branch changed from u/jdemeyer/ticket/24298 to 3479550c0e9d32362144072855fd116f41dc4220
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.