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

Do you still need the MapIter? #1506

Open
seberg opened this issue Jul 27, 2023 · 5 comments
Open

Do you still need the MapIter? #1506

seberg opened this issue Jul 27, 2023 · 5 comments

Comments

@seberg
Copy link
Contributor

seberg commented Jul 27, 2023

I was wondering if I can get rid of the MapIter as public API in NumPy thinking that Theano is end-of-life, but Aesara clearly isn't :).

Now, in new NumPy versions simple np.add.at() calls are vastly faster than manual MapIter use. But there are two problems:

  1. The better speed is relatively new (1.25 probably).
  2. It doesn't cover all paths, and the rest of the paths are still terrible so your version is probably at least 5x faster for those for the others, ours is probably 2-5x faster (speed factors are just wild guesses).

So, I guess this is just an FYI that at some point replacing the MapIter use with a direct np.add.at call is probably vastly better. I thought theano also had an advanced-indexing equivalence (which should just use advanced indexing). But I don't see it, so maybe that is already replaced.

@brandonwillard
Copy link
Member

Thanks for the heads-up!

Just to make sure I'm on the same page, you're referring to our uses of PyArrayMapIterObject in the C backend, right?

@seberg
Copy link
Contributor Author

seberg commented Jul 27, 2023

Right, I was somewhat hoping to just reduce the burden of keeping the ABI around. It isn't a big deal to keep it alive, though!

EDIT: There is an issue of bumping the maximum numbre of supported dimensions, although maybe I can pull it off for selected iterators (like this one) by hiding a few fields behind macros, in which case you probably need not do anything.

@brandonwillard
Copy link
Member

Right, I was somewhat hoping to just reduce the burden of keeping the ABI around. It isn't a big deal to keep it alive, though!

Yeah, I completely understand. We've been slowly deprecating all that code anyway, so don't let it complicate things for you. If you want to remove it, I'll start making the appropriate updates as soon as I can.

@seberg
Copy link
Contributor Author

seberg commented Nov 13, 2023

OK, I decided to push for this in NumPy, because there is also the wish to bump to 64 dimensions. And leaving it public would make it hard to make advanced indexing 64bit dimension compatible. (Not everything will be for the same reason, but here it just seems enough to end up on the side of removal.)

@brandonwillard
Copy link
Member

OK, I decided to push for this in NumPy, because there is also the wish to bump to 64 dimensions. And leaving it public would make it hard to make advanced indexing 64bit dimension compatible. (Not everything will be for the same reason, but here it just seems enough to end up on the side of removal.)

No problem, and thanks again for the update!

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

No branches or pull requests

2 participants