Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Accessor Functionality for AnnData #1870
base: main
Are you sure you want to change the base?
Accessor Functionality for AnnData #1870
Changes from 11 commits
790b211
105b155
a87a8e9
59d545b
8fa883d
f673c2b
9f6dc2a
00cff10
0121106
c85f38b
db2844b
16b49bc
657aa07
035df3d
f658fdd
ec4c9b7
db71c52
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a
typing.py
file so you don't have to reexportThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this resolved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot to remove it from there. Should the protocol go in
src/anndata/_types.py
orsrc/anndata/typing.py
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@flying-sheep can answer this best, but I think
_types.py
actuallyCheck warning on line 78 in src/anndata/_core/extensions.py
src/anndata/_core/extensions.py#L78
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the blanket
except
here with something more preciseCheck warning on line 155 in src/anndata/_core/extensions.py
src/anndata/_core/extensions.py#L154-L155
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more
type
usage that shoudl beExtensionNamespace
(there are others as well)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would think we want to disallow this behavior in some form. I wouldn't want someone overriding
X
. Maybe I'm misunderstanding, but I also don't see a test for thisThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, just tested it out and it absolutely overrides X. What attributes should be protected, should I just do all of them with
dir(AnnData)
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless there's a reason to allow overriding already existing attributes, why not just throw an attribute error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this was resolved but it's still just a warning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When using Jupyter notebooks, raising an
AttributeError
for overriding custom namespaces can be disruptive. When you modify and re-register a namespace, the user has to frequently restart their kernel to reset the accessor namespaces, because the namespace already exists. While protecting against conflicts with core AnnData attributes (likeX
,obs_names
, etc...) is good, being able to overwrite existing custom namespaces would be nice for iterative development in notebooks.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But as things stand, won't this function simply warn if you try to override
X
i.e.,elif hasattr(cls, name)
?