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 topk / maxk #352

Open
CarloLucibello opened this issue Sep 30, 2021 · 7 comments · May be fixed by #353
Open

add topk / maxk #352

CarloLucibello opened this issue Sep 30, 2021 · 7 comments · May be fixed by #353

Comments

@CarloLucibello
Copy link
Member

We should add what is called elsewhere topk or maxk, e.g. https://pytorch.org/docs/stable/generated/torch.topk.html

In Base we have partialsort and partialsortperm doing something similar but limited to 1d arrays

@CarloLucibello CarloLucibello linked a pull request Sep 30, 2021 that will close this issue
3 tasks
@DhairyaLGandhi
Copy link
Member

DhairyaLGandhi commented Sep 30, 2021

Those don't strictly appear to be NNlib's area to address though. We have topk in Metalhead.jl iirc

@DhairyaLGandhi
Copy link
Member

We could also steal them from ResNetImageNet.jl

@CarloLucibello
Copy link
Member Author

It's generally useful. Pytorch and tensorflow have it, deep graph library and pytorch geometric use it internally and I need it for https://github.com/CarloLucibello/GraphNeuralNetworks.jl. I'll look into and eventually port here those other implementations

@CarloLucibello
Copy link
Member Author

I can't find topk in Metalhead.jl, nor in ResNetImageNet.jl

@DhairyaLGandhi
Copy link
Member

https://github.com/FluxML/Metalhead.jl/blob/23df74c7df1bfca2ee66392a29bc3529deab9dbf/src/utils.jl#L73 (these functions should be ported back to master)

https://github.com/DhairyaLGandhi/ResNetImageNet.jl/blob/03497c26879d7462c1bb3c6199f54738c758fb84/src/utils.jl#L20

I agree that these are generally useful, but not a "neural network primitive". I can see it in a higher abstracted library like Metalhead, maybe Flux.

@darsnack
Copy link
Member

I don't think they belong in Metalhead.jl as they are not specific to vision models. The advantage of NNlib is that packages unrelated to Flux can use it. topk is in the same category as losses for me, so Flux seems like the right place.

@CarloLucibello
Copy link
Member Author

I consider it a differentiable transformation, a generalization of maximum, and prefer having it here for wider availability. We also have softmax here, which is a bit related as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants