search

aesara-devs / aesara

Aesara is a Python library for defining, optimizing, and efficiently evaluating mathematical expressions involving multi-dimensional arrays.
https://aesara.readthedocs.io
Other
1.18k stars 155 forks source link

Do you still need the MapIter? #1506

Open seberg opened 1 year ago

seberg commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 10 months ago

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 commented 10 months ago

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!