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

Include an integer setindex for NamedTuple #43155

Open
rafaqz opened this issue Nov 19, 2021 · 13 comments
Open

Include an integer setindex for NamedTuple #43155

rafaqz opened this issue Nov 19, 2021 · 13 comments
Labels
domain:collections Data structures holding multiple items, e.g. sets good first issue Indicates a good issue for first-time contributors to Julia

Comments

@rafaqz
Copy link
Contributor

rafaqz commented Nov 19, 2021

We have both Symbol and Int getindex for a NamedTuple, but only Symbol setindex.

Is there a reason for this? its seems like we should have both in both cases.

@JeffBezanson JeffBezanson added the good first issue Indicates a good issue for first-time contributors to Julia label Nov 19, 2021
@Xnartharax
Copy link

Hi, I would like to work on this if no one else is currently on it. That would be my first real contribution to an open source project.

@Xnartharax
Copy link

So my proposed method is something like this:

function setindex(nt::NamedTuple{names, T}, v, idx::Int)
     NamedTuple(zip(names, setindex(Tuple(nt), v, idx)))
end

@rafaqz
Copy link
Contributor Author

rafaqz commented Dec 5, 2021

You want to make sure that is type stable. I would use names in the constructor i.e NamedTuple{names}(...)

And check what is commonly used for names in other methods in Base.

@Xnartharax
Copy link

The method you suggested unfortunately doesn't work and will return the following:

julia> Base.setindex((a=1, b=2), 3, 1)
(a = (:a, 3), b = (:b, 2))

@Xnartharax
Copy link

Xnartharax commented Dec 5, 2021

However after some experimentation I noticed this will work:

function setindex(nt::NamedTuple{names, T}, val::X, idx::Int) where {names, T, X}
      NamedTuple{names}(Base.setindex(Tuple(nt), val, idx))
end

This is also an order of magnitude faster. Thanks for the suggestion will update my pull request.
Also, names is the common name for this parameter.

@rafaqz
Copy link
Contributor Author

rafaqz commented Dec 5, 2021

Yes, that's what I mean ;).

Also it shouldnt be "faster" it should be essentially a compile-time operation with no allocations. Anything else shouldnt be merged.

@Xnartharax
Copy link

Also it shouldnt be "faster" it should be essentially a compile-time operation with no allocations.

I'm not sure what you mean by this. setindex requires at least the allocation of the new named tuple since it is not an inplace function.

@rafaqz
Copy link
Contributor Author

rafaqz commented Dec 5, 2021

NamedTuple is allocated on the stack, or not at all.

Allocations that show in benchmarks are heap allocations. Thats what we want to be zero.

@Xnartharax
Copy link

Thanks for that insight and yes indeed benchmark shows 0 heap allocations.

@Hitansh-Shah
Copy link

Hi! I am new to Julia and I want to contribute to the Julia community. I completed the Julia basics course from JuliaAcademy. I am also looking forward to Gsoc'22 with Julia. Can anyone guide on how I can start contributing to the good first issues because as a beginner in Julia they seem pretty overwhelming too. 😅

@srvjha
Copy link

srvjha commented Mar 7, 2023

hey I want to contribute to this , could u please assign me

@oscardssmith
Copy link
Member

We don't generally assign issues. Make a PR and we'll review it.

@rafaqz
Copy link
Contributor Author

rafaqz commented Mar 7, 2023

Also the code from @Xnartharax is pretty much all of it. The real work is writing tests and docs.

@nsajko nsajko added the domain:collections Data structures holding multiple items, e.g. sets label Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:collections Data structures holding multiple items, e.g. sets good first issue Indicates a good issue for first-time contributors to Julia
Projects
None yet
Development

No branches or pull requests

7 participants