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

Update documentation for EnumeratedFamily #34054

Closed
trevorkarn opened this issue Jun 22, 2022 · 21 comments
Closed

Update documentation for EnumeratedFamily #34054

trevorkarn opened this issue Jun 22, 2022 · 21 comments

Comments

@trevorkarn
Copy link
Contributor

The current documentation implies that the index set must be finite. This is not the case:

sage: F = Family(ZZ)
sage: type(F)
<class 'sage.sets.family.EnumeratedFamily_with_category'>

CC: @tscrim

Component: documentation

Keywords: gsoc2022 documentation family

Author: Trevor K. Karn

Branch/Commit: c777eca

Reviewer: Matthias Koeppe

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

@trevorkarn trevorkarn added this to the sage-9.7 milestone Jun 22, 2022
@tscrim
Copy link
Collaborator

tscrim commented Jun 23, 2022

comment:2

I am not sure the documentation is wrong, but it might be a slight abuse of notation. In particular |ZZ| is aleph0, and aleph0 - 1 = aleph0 (well, this might make slightly more sense in terms of ordinals rather than cardinals). So it is saying it is indexed by {0, 1, ...}.

Now your change definitely is wrong because it is not indexed by Z but by N (with 0) in the infinite case. Don't let this run for too long:

sage: F = Family(ZZ)
sage: F[-1]

as the generic unrank() from the category doesn't protect against bad input (for infinite enuemrated sets, IMO, it should).

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 10, 2022

Changed commit from d4dedf6 to 79250c6

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 10, 2022

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

79250c6Add documentation of Integer index

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 10, 2022

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

f11c1a3Add check for positivity of rank

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 10, 2022

Changed commit from 79250c6 to f11c1a3

@trevorkarn
Copy link
Contributor Author

comment:5

I changed this to reflect your comment about ZZ[-1], and then saw that _unrank_from_iterator expects a positive rank, but does not check for it, so I added the check there.

@tscrim
Copy link
Collaborator

tscrim commented Aug 10, 2022

comment:7

There is no reason for a separate TESTS:: block. I would also put the ZZ test below the C one to have better grouping of the tests.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 10, 2022

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

250ee34Remove TESTS block

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 10, 2022

Changed commit from f11c1a3 to 250ee34

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 16, 2022

comment:9
+            if r < 0:
+                raise ValueError("the value must be greater than or equal to 0")

I'd suggest to change from "value" to "rank" here.

Otherwise LGTM

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 16, 2022

comment:10

Maybe it would also be a good idea to check that it's an integer?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 17, 2022

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

19f5820value->rank and check for integer value

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 17, 2022

Changed commit from 250ee34 to 19f5820

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 17, 2022

comment:12

I would rephrase "must be between %s and %s inclusive" -> "must be in the range from %s to %s"

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 17, 2022

comment:13

I know that this is not from your ticket, but this:

             counter = 0
             for u in self:
                 if counter == r:
                     return u
                 counter += 1

could be rewritten using enumerate

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 17, 2022

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

cd816e6Rephrase range

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 17, 2022

Changed commit from 19f5820 to cd816e6

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 17, 2022

Changed commit from cd816e6 to c777eca

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 17, 2022

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

c777ecaRewrite using enumerate and fix doctests

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 18, 2022

Reviewer: Matthias Koeppe

@vbraun
Copy link
Member

vbraun commented Aug 30, 2022

Changed branch from u/tkarn/enumerated-family-ZZ-doc to c777eca

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

4 participants