-
-
Notifications
You must be signed in to change notification settings - Fork 482
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
Categories for Family #34340
Comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Commit: |
New commits:
|
This comment has been minimized.
This comment has been minimized.
Branch pushed to git repo; I updated commit sha1. New commits:
|
Branch pushed to git repo; I updated commit sha1. New commits:
|
Branch pushed to git repo; I updated commit sha1. New commits:
|
Branch pushed to git repo; I updated commit sha1. New commits:
|
Author: Matthias Koeppe |
This comment has been minimized.
This comment has been minimized.
Branch pushed to git repo; I updated commit sha1. New commits:
|
This comment has been minimized.
This comment has been minimized.
comment:14
The main part of your changes LGTM, it is good to have this extra flexibility. I just have some issues with the docstring you changed. For the first paragraph, I am not a fan of giving implementation details to the more casual user in the docstring. If they care about such details, then they almost certainly are already looking at the code. I know it was there previously, but now it has been given more prominence. So I think the first paragraph about the function being a factory should go last (although I really would like to remove it altogether). The second paragraph is good (I am not counting the one-line description as a paragraph). I don't really understand the third paragraph, even what you are trying to say. My thought is that it will be far too technical and likely should be removed. Although it would be worthwhile to state that iterating through the object returns the values rather than the keys. I don't understand the second part of the fourth paragraph. I think I know what you are trying to say, but that is not anything that belongs in this documentation. It seems like a code comment for within the category. I don't think the first sentence is even necessary (this is true for nearly every object in Sage). I would change |
comment:15
It boils down to this: When the function is not injective, there is a subtle conflict between methods such as |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:17
Unfortunately I don't see how that is in the documentation (beyond the
The |
Branch pushed to git repo; I updated commit sha1. New commits:
|
Branch pushed to git repo; I updated commit sha1. New commits:
|
Branch pushed to git repo; I updated commit sha1. New commits:
|
Work Issues: Add method "as_map" |
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
|
This comment has been minimized.
This comment has been minimized.
comment:143
|
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
|
Removed branch from issue description; replaced by PR #35175 |
Currently a
LazyFamily
is automatically inEnumeratedSets
, so at most countable.We add keyword
category
to the constructorFamily
and all implementation classes, so that the user can choose the correct category.Moreover, we define a (non-full) subcategory of
Sets
namedFamilies
, which captures the additional structure (= the map from the keys onto the parent).Follow-ups / related:
function
from indices to objects is a morphism, then the category can be taken from the codomain.Depends on #34375
CC: @tscrim
Component: combinatorics
Work Issues: Add method "as_map"
Author: Matthias Koeppe
Issue created by migration from https://trac.sagemath.org/ticket/34340
The text was updated successfully, but these errors were encountered: