added haskey method for tuple

[uluviano]

#35516
I added the suggested fallback and some tests to make sure it works with both Int and Float keys.
This is my first PR, so feel free to make any comment you might consider relevant.

I was also very confused about the @_inline_meta macro, but included it since similar functions had it. I tried googling something about it but found very cryptic explanations.

All in all, I hope this is a useful contribution :)

[uluviano]

Hi! I tried to read the error messages from the 2 builds that failed but I couldn't understand if they're related to the changes I did. (I'd suppose they are, but I cannot find the function I defined anywhere. Maybe I should define the types of the function explicitly in order for the method I defined not to be used in other places.

[StefanKarpinski]

Note that this definition is far more general than just tuples. @JeffBezanson: are you ok with this kind of highly general fallback definition?

[uluviano]

Hi! If not, I could define it instead as
haskey(container::Tuple, key)
do you think this generality is responsible for the builds not passing?

[StefanKarpinski]

No, the builds aren't passing for a variety of unrelated reasons that are being worked on.

[uluviano]

Ok! :) So I'll wait for @JeffBezanson to express his opinion on the typing of the container and maybe update the branch. Does that sound alright?

[JeffBezanson]

Welcome; thanks for making this PR!

I think I would rather have a more specific haskey(t::Tuple, i::Real) method. Very general methods like this have a few issues:

• Reduces the quality of hasmethod queries, since every object appears to have the method
• It's not obvious that in and keys are more fundamental than haskey
• It is potentially O(n)
• Harder on the compiler, since when the type of the first argument is uncertain it might have to examine this method even though it's unlikely to be called (because a more specific method is probably also applicable).

We should also discuss a bit what haskey actually means. Notably, there isn't a method for arrays either. Both arrays and tuples can also be indexed with CartesianIndex, so those methods should maybe be defined as well. In that case, the general definition here might also be considered incorrect.

[uluviano]

Hi! Thanks a lot for taking the time to explain what's the deal with the possible problems of having such a general method. I'm not so sure why it could be O(n) (And how would that change with a more specific method).

In that case, the general definition here might also be considered incorrect.

I tried applying the function to an array and it seems to work; I think I probably don't understand what you meant by that last sentence.

[JeffBezanson]

Some collections allow different keys for the same element, for example

julia> a = [1,2];

julia> a[1]
1

julia> a[CartesianIndex(1)]
1

but keys only returns one kind of key:

julia> CartesianIndex(1) in keys(a)
false

The same thing happens with NamedTuples, which allow both integers and symbols.

[uluviano]

Interesting. Then we should also add a method for key::CartesianIndex{1} ?

[vtjnash]

Closing, as this seems stale, though still welcome. Jeff gave good feedback above that this would need to explicitly handle more cases, instead of being a catch-all.

[uluviano]

Hi! I got a little bit confused with the workflow.
I'd like to implement Jeff's suggestion to handle explicitly the other cases. Should I open a new PR?

[vtjnash]

I can reopen this one (until you push a new commit, then github will prohibit reopening), or you can make a new PR and reference this one as the description. Either is great :)

[uluviano]

I'm sorry. This is my first PR so things are a bit new to me. I think reopening this one would be easier, but I didn't understand if I need to push a new commit or if pushing a new commit will make it impossible to reopen it.

[uluviano]

Thanks for reopening the branch. Since v1.6 just came out, it would make sense to pull that first, just to make sure everything still works, right?

[uluviano]

Hi! I have a little question for @JeffBezanson (or anybody else who'd like to help me out) . It might be a bit silly, but this being my first PR I'd rather be sure.

I'm trying to implement a method for key::CartesianIndex{1} like this:

function haskey(container::Tuple, key::CartesianIndex{1})
    @_inline_meta
    key[1] ∈  keys(container)
end

but when I run make test-tuple I get this error:

error during bootstrap:
LoadError(at "compiler/compiler.jl" line 3: LoadError(at "tuple.jl" line 90: UndefVarError(var=:CartesianIndex)))
...

I suppose it's because tuple.jl doesn't know of CartesianIndex before hand, so I'd like to know what's the cleanest way of importing the definition in tuple.jl

Thanks in advance!

[vtjnash]

I would suggest putting that method after the definition of CartesianIndex, rather than trying to make it work in tuples.jl

[uluviano]

That makes a lot of sense. Thanks! :)

Once that's done, I should maybe rebase and squash some commits before pushing it here, right?

[uluviano]

Hi!
I defined the haskey method for CartesianIndex{1} in multidimensional.jl but I can't seem to find a good place for the tests. I added them in cartesian.jl but when I run make test-cartesian I get errors that say that there's no method for CartesianIndex{1}, as if the method I defined had not been imported. Maybe I'm getting confused with the way the modules work.
I would appreciate some help.

Thanks in advance :)