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
index_labels
method andIndexLabel
type indexing #328index_labels
method andIndexLabel
type indexing #328Changes from 10 commits
aa8bdf9
88be1b1
8f1e165
2e4e4a2
d05d7bc
5f7434f
c27eb5a
b8d6e93
139fb33
8fde10b
d6cb218
bc7a635
7231dbe
268576f
122536f
d7c0a27
809839b
0ad5cd6
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.
Is it really a good idea to have the default return
keys
? I'd say better returnnothing
so that the caller knows that no labels are set (it's easy to callkeys
if needed). Otherwise there's no way to know whether labels are present or not.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 should probably have changed that back to just the axis after all the discussion around naming this. My thinking was that it would be odd if we returned something that didn't "label" the indexes at all because we are assuming that is what's returned.
Returning
nothing
would require that every method that acts on index labels would require and extra step to check if it can actually look up values in it. Not an insurmountable issue, but it would certainly be tedious.Throwing an error when this isn't defined also isn't an option because things like a vector transpose will inevitably have a dimension without labels.
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 we don't return
nothing
when no labels are set, then we should add a separate function called e.g.has_index_labels
, a bit like we did in the Metadata PR withhasmetadata
andmetadata
.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.
That's definitely doable and is probably a good method to have irregardless of what the default we choose.
I'm not against returning something that clearly shows no labels are defined. Perhaps I'm being too strict with the rule that the return has the same indices as their corresponding axis, even for default values. I assume people will at least expect to be able to do things like
findfirst(==(label), index_labels(A, dim))
, in which case we would probably still want a more informative 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 would also prefer
nothing
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.
Is it really worth defining a special type just for that? What would be the advantage over
Nothing
?If in doubt, we could say that an error is thrown if
has_index_labels
returnsfalse
, as it allows doing anything we want later without breaking code.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'm just trying to figure out some generic representation of labels not being defined that can pass between methods. If we do
vcat(a, b)
but one doesn't have index labels, then we can't dovcat(index_labels(a, 1), index_labels(b, 1))
. Even if we define something that doesn't error in this situation (e.g.,vcat_index_labels(::AbstractVector, ::Nothing)
), how does this produce a new vector of index labels for the newly generated array? I figuredUndefLabel
could represent every index without a label.@rafaqz how are you handling this situation?
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.
@rafaqz, it looks like
NoLookup
is basically the same thing as what I'm describing. It wraps the non-labeled indices.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.
Also, do we want to have
has_index_labels(T::Type)
orhas_index_labels(x) = any(are_labels, index_labels)
?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.
Sorry I missed this earlier. If your
UndefLabels
is essentially myNoLookup
, that should be fine.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.
Shouldn't the function live in ArrayInterfaceCore so that existing "named array" packages can overload it? BTW, it would be good to ping their authors to ensure they would all be OK with the API, otherwise it won't make a lot of sense.
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.
Existing packages can overload it from
ArrayInterface
. If you're referring to supporting named dimensions like NamedDims.jl, then they defineArrayInterface.dimnames
andto_dims
maps to the appropriate dimension so they don't have to overload every method with a dim argument.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.
ArrayInterface is relatively heavy, which is why ArrayInterfaceCore was created IIUC. I guess it's up to package authors to say whether a dependency on ArrayInterface is acceptable for them or not.
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.
That was more of an issue before we went through all the trouble to fix invalidations due to
StaticInt
earlier this year. That doesn't mean we can't improve the situation. We are actively trying to move matured functionality into base where appropriate (see #340). I regularly review the code here in an effort to eliminate problematic code that still exists (e.g., redundancies, generated functions, etc,). For example, once we know how this PR is going to look I can finally finish an effort to consolidate a lot of "indexing.jl" by overloading base methods instead of reimplementing a lot of what's in base already.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.
Also, it's a bit of a chicken and egg issue. We use
StaticSymbol
for dimension names known at compile time so that we can use them as a point of reference in an inferrible way. It's pretty difficult to do this only relying on constant propagation (demonstrated that with static sizes here JuliaLang/julia#44538 (comment)).If someone has a reliable solution I'm open to it. I've been trying to actively address this and related issues for years
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.
@oxinabox is it still a concern depending on ArrayInterface at this point?
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.
Now that ArrayInterface doesn't like Requires.jl a billion packages, I am much more comfortable depending upon it for NamedDims.jl
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.
My suggestion was just to put empty function definitions in ArrayInterfaceCore (like we do with DataAPI and StatsAPI). That doesn't prevent keeping fallback method definitions in ArrayInterface as this PR does. But packages that don't want to use fallback definitions are still able to overload the functions by depending only on ArrayInterfaceCore.
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've been trying to avoid situations where the behavior of a method is different if ArrayInterface is loaded vs just ArrayInterfaceCore.