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
FeatureStore
abstraction definition #4534FeatureStore
abstraction definition #4534Changes from 3 commits
3fec3c6
e42ae42
e962338
b347a3c
a0486b9
e602325
e4f4f42
99b625a
832802a
46b76de
65e5e0b
6fdf062
94369fe
e0c63c9
0fd6dd2
94d50df
385512e
a5c4f1c
bc2b0e3
f949207
bff0bf1
b2064d5
23d4e58
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.
getattr
andsetattr
equivalents?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 don't think a
FeatureStore
should implementgetattr
andsetattr
; imo, these should only be implemented for views on the store. I don't think it's particularly clean to haveas this syntax seems more confusing to me than clarifying.
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.
Thought it makes sense to implement for stores without group names (like PyG
data
objects). We could require that the output is fully specified in case we allow it.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.
Isn't the only way for an output to be fully specified through
getattr
to allow for chaining, which necessitates thatgetattr
can return anAttrView
? That feels odd to me.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.
If
TensorAttr
sets bothgroup_name
andindex
toNone
by default, thenstore.{attr_name}
should give you a tensor. I am okay with leaving this out for now.