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

Implement np.mgrid, np.ogrid, & np.r_, np.c_ #5850

Closed
6 tasks done
Conchylicultor opened this issue Feb 25, 2021 · 18 comments
Closed
6 tasks done

Implement np.mgrid, np.ogrid, & np.r_, np.c_ #5850

Conchylicultor opened this issue Feb 25, 2021 · 18 comments
Labels
enhancement New feature or request good first issue Good for newcomers open Issues intentionally left open, with no schedule for next steps. P3 (no schedule) We have no plan to work on this and, if it is unassigned, we would be happy to review a PR

Comments

@Conchylicultor
Copy link
Member

Conchylicultor commented Feb 25, 2021

Numpy has a very convenient np.mgrid which allow to easily create coordinate grids: https://numpy.org/doc/stable/reference/generated/numpy.mgrid.html.

I was expecting jax to have the same, but it seems to be missing:

jnp.mgrid  # raise AttributeError

Tracking implementation of these:

@Conchylicultor Conchylicultor added the enhancement New feature or request label Feb 25, 2021
@jakevdp
Copy link
Collaborator

jakevdp commented Feb 25, 2021

Thanks! Agreed it would be useful to have a jax-native version of this. ogrid and r_ are similarly missing.

@jakevdp jakevdp added the good first issue Good for newcomers label Feb 25, 2021
@jakevdp jakevdp changed the title np.mgrid missing in jax Implement np.mgrid, np.ogrid, & np.r_ Feb 25, 2021
@VikasHanumegowda
Copy link

I'd love to solve this. Anyone with a good direction suggestion for me to point to, to solve this issue? @jakevdp @Conchylicultor

@Conchylicultor Conchylicultor changed the title Implement np.mgrid, np.ogrid, & np.r_ Implement np.mgrid, np.ogrid, & np.r_, np.c_ Feb 26, 2021
@Conchylicultor
Copy link
Member Author

To add to the list, np.c_ is missing too: https://numpy.org/doc/stable/reference/generated/numpy.c_.html#numpy.c_

@jakevdp
Copy link
Collaborator

jakevdp commented Feb 26, 2021

The way to implement these would be to create a private class, and override the __getitem__ method to mimic what numpy does. Something like this:

import jax.numpy as jnp

class _Mgrid:
  def __getitem__(self, key):
    if isinstance(key, slice):
      return jnp.arange(key.start or 0, key.stop, key.step or 1)
    else:
      raise NotImplementedError()

mgrid = _Mgrid()
print(mgrid[:5])
# [0 1 2 3 4]

The challenge will be to make certain it handles all the input cases that np.mgrid does.

@Conchylicultor
Copy link
Member Author

You could also look at the numpy implementation to see how they are implementing it

@jakevdp
Copy link
Collaborator

jakevdp commented Feb 26, 2021

You could also look at the numpy implementation to see how they are implementing it

But note that if you copy from numpy's implementation rather than writing your own version, the code should go in https://github.com/google/jax/tree/master/jax/_src/third_party for licensing reasons.

@hawkinsp
Copy link
Member

And in this case, where there's probably no need to look at NumPy's implementation to implement the API faithfully and well, it's much better to implement from scratch so we don't have to split up the code based on license. In general:

  • please try not to use code from NumPy
  • if you do use code from NumPy, it must go in third_party/, and ultimately may need to be rewritten anyway.

@jakevdp jakevdp added the open Issues intentionally left open, with no schedule for next steps. label Mar 2, 2021
@minoring
Copy link
Contributor

Is there an any update for this issue? If not, I would like to progress it :)

@VikasHanumegowda
Copy link

I didn't find time for this @minoring
Please go ahead.

@minoring
Copy link
Contributor

Thanks for your response! @VikasHanumegowda

minoring added a commit to minoring/jax that referenced this issue Mar 27, 2021
minoring added a commit to minoring/jax that referenced this issue Mar 27, 2021
minoring added a commit to minoring/jax that referenced this issue Mar 31, 2021
minoring added a commit to minoring/jax that referenced this issue Mar 31, 2021
minoring added a commit to minoring/jax that referenced this issue Mar 31, 2021
minoring added a commit to minoring/jax that referenced this issue Apr 1, 2021
minoring added a commit to minoring/jax that referenced this issue Apr 2, 2021
@SondosAhmed
Copy link

Hi
I'd like to help with this issue if possible
any update?

@jakevdp
Copy link
Collaborator

jakevdp commented Apr 3, 2021

mgrid has been implemented in #6248, but I don't know of any other work on this

@minoring
Copy link
Contributor

minoring commented Apr 3, 2021

Hi!
I was working on implementing jnp.ogrid.
Maybe you can implement jnp.r_ so that we do not conflict to each other :)

@SondosAhmed
Copy link

SondosAhmed commented Apr 3, 2021

@jakevdp @minoring Thanks for informing me
I'm gonna try implementing jnp.r_

minoring added a commit to minoring/jax that referenced this issue Apr 5, 2021
minoring added a commit to minoring/jax that referenced this issue Apr 5, 2021
minoring added a commit to minoring/jax that referenced this issue Apr 5, 2021
NeilGirdhar pushed a commit to NeilGirdhar/jax that referenced this issue Apr 7, 2021
NeilGirdhar pushed a commit to NeilGirdhar/jax that referenced this issue Apr 7, 2021
@arv-77
Copy link

arv-77 commented May 18, 2021

Can I take up jnp.s_ if no one else is working on this.

@jakevdp
Copy link
Collaborator

jakevdp commented May 18, 2021

I don't know of anyone working on jnp.s_ – feel free to dive in!

@apaszke apaszke added the P3 (no schedule) We have no plan to work on this and, if it is unassigned, we would be happy to review a PR label Jun 2, 2021
@lgeiger
Copy link
Contributor

lgeiger commented Aug 16, 2021

Looks like this issue can be closed since all function have been implemented 🚀
np.s_ was the last remaining function which was added in #7114.

@jakevdp
Copy link
Collaborator

jakevdp commented Aug 16, 2021

Thanks!

@jakevdp jakevdp closed this as completed Aug 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers open Issues intentionally left open, with no schedule for next steps. P3 (no schedule) We have no plan to work on this and, if it is unassigned, we would be happy to review a PR
Projects
None yet
Development

No branches or pull requests

9 participants