Skip to content
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

add note on eltype and getindex #39204

Closed
wants to merge 2 commits into from

Conversation

kalmarek
Copy link
Contributor

following discussion in #38422
@nalimilan

Comment on lines 177 to 178
`eltype(typeof(itr)) == typeof(first(itr))` and if `itr` implements `Base.getindex`, then also
`eltype(typeof(itr)) == typeof(itr[a_valid_index])` holds.
Copy link
Member

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.

Copy link
Contributor Author

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))?

Copy link
Sponsor Member

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.

Copy link
Sponsor Member

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)

Copy link
Member

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.

Copy link
Contributor Author

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...

Copy link
Sponsor Member

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.

Copy link
Contributor Author

@kalmarek kalmarek Jan 13, 2021

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.

Copy link
Sponsor Member

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.

Copy link
Contributor Author

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.

@mbauman mbauman added the docs This change adds or pertains to documentation label Jan 12, 2021
@vtjnash
Copy link
Sponsor Member

vtjnash commented Apr 3, 2021

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.

@vtjnash vtjnash closed this Apr 3, 2021
@kalmarek
Copy link
Contributor Author

kalmarek commented Apr 5, 2021

@vtjnash I'm happy to work on this, but there doesn't seem to be a consensus on what is actually required for eltype.

with #39295 there will be a need to implement eltype equal to the type inferred by getindex to keep broadcasting the same as manual loop(?). I'm happy to return to this pull then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This change adds or pertains to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants