Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improvements to ImageSet #34377

Closed
mkoeppe opened this issue Aug 16, 2022 · 18 comments
Closed

Improvements to ImageSet #34377

mkoeppe opened this issue Aug 16, 2022 · 18 comments

Comments

@mkoeppe
Copy link
Member

mkoeppe commented Aug 16, 2022

(split out from #34340)

We equip ImageSet with an _element_constructor_ and hence, with a membership test. (This needs a map with inverse.)

We also add a new parameter value is_injective='check'.

CC: @tscrim

Component: combinatorics

Author: Matthias Koeppe

Branch/Commit: 6aeab7d

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/34377

@mkoeppe mkoeppe added this to the sage-9.7 milestone Aug 16, 2022
@mkoeppe
Copy link
Member Author

mkoeppe commented Aug 16, 2022

Branch: u/mkoeppe/improvements_to_imageset

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 16, 2022

Commit: 52f6ca6

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 16, 2022

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

52f6ca6src/sage/sets/image_set.py: Improve example

@mkoeppe

This comment has been minimized.

@tscrim
Copy link
Collaborator

tscrim commented Aug 17, 2022

comment:4

I think you should try-except the call to self._inverse(x) as that could fail in many different ways (with the actual error getting lost in the noise). Subsequently, I am not sure of the utility of verifying that it is an inverse versus trusting the user did give an honest inverse.

There is also an infinite recursion that seems to be happening:

src/sage/categories/sets_cat.py  # 1 doctest failed

Also, calling len(list(self)) might run for forever if self is infinite. I think list(self) also does a __len__ check too. This is also probably related to the infinite recursion above.

@mkoeppe
Copy link
Member Author

mkoeppe commented Aug 17, 2022

comment:5

Replying to @tscrim:

I think you should try-except the call to self._inverse(x) as that could fail in many different ways (with the actual error getting lost in the noise).

Yes, I think that's a good idea

Subsequently, I am not sure of the utility of verifying that it is an inverse versus trusting the user did give an honest inverse.

Mapping back is necessary to get an element that is in the correct parent.
The comparison is just a safety check there.

@tscrim
Copy link
Collaborator

tscrim commented Aug 17, 2022

comment:6

Replying to @mkoeppe:

Replying to @tscrim:

Subsequently, I am not sure of the utility of verifying that it is an inverse versus trusting the user did give an honest inverse.

Mapping back is necessary to get an element that is in the correct parent.
The comparison is just a safety check there.

Couldn't we just do self.codomain()(x) for this?

@mkoeppe
Copy link
Member Author

mkoeppe commented Aug 17, 2022

comment:7

If it's an honest Map, yes, but this is supposed to work also for PoorManMap

@tscrim
Copy link
Collaborator

tscrim commented Aug 17, 2022

comment:8

Ah, right. Thanks.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 17, 2022

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

9e8d023ImageSubobject.cardinality: Handle infinite cardinalities explicitly

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 17, 2022

Changed commit from 52f6ca6 to 9e8d023

@mkoeppe
Copy link
Member Author

mkoeppe commented Aug 17, 2022

comment:10

Bots are green now

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 17, 2022

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

6aeab7dsrc/sage/sets/image_set.py: Clarify is_injective=False

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 17, 2022

Changed commit from 9e8d023 to 6aeab7d

@tscrim
Copy link
Collaborator

tscrim commented Aug 20, 2022

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Aug 20, 2022

comment:12

LGTM.

@mkoeppe
Copy link
Member Author

mkoeppe commented Aug 20, 2022

comment:13

Thanks.

@vbraun
Copy link
Member

vbraun commented Aug 30, 2022

Changed branch from u/mkoeppe/improvements_to_imageset to 6aeab7d

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants