-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
add note on eltype and getindex #39204
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
Conversation
following discussion in JuliaLang#38422
base/abstractarray.jl
Outdated
`eltype(typeof(itr)) == typeof(first(itr))` and if `itr` implements `Base.getindex`, then also | ||
`eltype(typeof(itr)) == typeof(itr[a_valid_index])` holds. |
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 that is correct. The latter just has to be a subtype of the former.
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 learned about these assumptions when broadcasting on a struct of mine got broken in julia-1.6. Could you point me to any reference for typeof(first(itr)) <: eltype(typeof(itr))
?
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.
The later is not required, or expected, to hold true. The eltype
is for iteration, not indexing.
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.
What he means is that eltype may be any supertype, for example, it may be Any
. What is expected to hold true is:
all(isa(eltype(itr)), itr)
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.
Yes subtyping is enough. But why should eltype
apply to iteration but not indexing @vtjnash? I don't really care, but that sounds natural.
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.
Else, there are numerous counter-examples where the requirement doesn't hold, even for array (such as indexing with a BitArray or Vector)
ok,I thought that the docstring could say
`typeof(itr[idx]) <: eltype(typeof(itr)) ` holds for each `idx` in `eachindex(itr)`.
but indexable types do not necessarily implement eachindex
...
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.
This is largely already documented in https://docs.julialang.org/en/v1/manual/interfaces/ — but again, what's required is dependent upon the abstract super type.
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.
#39185 tries to document a case that was broken by 1.6 and does follow everything(?) what is in written in https://docs.julialang.org/en/v1/manual/interfaces/.
I'll wait for the conclusion of discussions in #39185 on what to write here exactly.
EDIT: the type in real life is already fixed (by introducing a type iterator struct), I just wanted to document the requirement. If the docstring of eltype
is not the place to document it, I'd be glad if you could suggest a better option.
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 didn't mean to imply that this shouldn't be documented here — just that the requirements depend upon the interface and it's hard to document exactly what this means without reference to this.
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.
@mbauman no offence was taken ;)
Are there any discussions here (github) or elsewhere on Number
interface? It seems that this issue should belong there.
I don't think this adds additional clarity over what is already stated there. Perhaps we could mention AbstractArray instead of just AbstractDict, and especially reference the interface documentation? Closing as I don't think we should merge this version, but feel free to re-open or to push a new PR with updates. |
following discussion in #38422
@nalimilan