Map with multiple inputs

[dvd101x]

Hi, this is intended to add multiple inputs to the function map.

Currently this includes:

• Mapping multiple arrays or matrices
• Broadcasts the input matrices if needed
• Implements the callback function with multiple inputs
• Optionally the callback function can have the broadcasted index and all the broadcasted arrays or matrices.
• Tests for the new functionality
• Fixes an issue with the current map transform function missing the datatype of the matrix.

It's missing:

• The transfer function for the multiple map functionality
• Tests for the transform function new functionality
• Documentation

Could you please give me advice on a recommended way to implement the transform function?

I have tried or studied the following alternatives:

• Create the map function inside transfer, but had some issues with the callback function.
• Implementing the full logic of the main function, as some other transform functions, which I think could work but I don't know if it's ok.
• Try to use the current transfer implementation and build on top on that with refer to self.

At this point I'm stuck so any pointers are greatly appreciated.

this is related to #3196

[josdejong]

Thanks for working this idea out in a PR David!

About the transform function: I think the transform function should only be a small wrapper around the actual implementation, and only adjust the index in the callback function from zero based to one based. So I would expect that we have to adjust the current (recursive) map function:

https://github.com/josdejong/mathjs/blob/develop/src/expression/transform/map.transform.js#L63-L66

from recurse(child, index.concat(i + 1)) into something like recurse(child1, child2, index.concat(i + 1)), but then probably something more generic like recurse(childs, index.concat(i + 1)). Did you gave that a try? If so, what issues did you came across when trying it?

[dvd101x]

Hi, thanks for the advice!

Currently recurse works for me, as it can recurse over the first array (broadcasted) and the callback function can take the argument of the rest (also broadcasted, thus the same size). So instead of changing recurse, it makes a special callback to make it work with the current recurse. It's not a pure function but it works. I have that logic on the main function, but I don't know if I should bring that code to the transform function. Your idea certainly looks cleaner.

Some issue that I found is that map.transform practically duplicates the code in map except that it adds one to the index, thus it's not a wrapper around the actual implementation. I could continue in that direction by copying the code but I'm not sure about it since the code is now longer.

By looking at other transform functions, I see in some cases those import the original function first, instead of duplicating the code. In the case of map.tranform why does it need to duplicate the code in map?

[dvd101x]

I think I'm on track again.

I'm working on a way to use the create map. My plan is to make a new callback that when the callback is a typed function it makes a new typed callback that for each signature changes the function associated with the signature to be zero based depending on the number of arguments of the function.

It might take a while but I think it might work. Just to let you know there is some progress.

[dvd101x]

Hi Jos,

I need your guidance again. What's a good way to get the number of arguments of f in the following example?

const f = math.evaluate("f(x, index) = index[1]")

I tried with f.signatures['any,any'].length but gives me 0 when I expect 2.

[josdejong]

Thanks for the updates.

You indeed cannot safely use f.signatures['any,any'].length. But if you have 'any,any', you could do 'any,any'.split(',').length for example? Not sure why you need it but I guess I'll see it in action in the PR :)

On a side note: since this is not easy to solve, let's stay critical: if this feature is very complex to implement (and consecutively: hard to maintain), let's think through if that is worth it.

[dvd101x]

Thanks Jos!,

I understand.

The main functionality is already working, it's currently just an issue with trying to fix the transform function without copying the logic and for that I think I need to transform the callback function with the right number of arguments. Maybe it makes more sense to continue copying the logic with a modification for the index than making new logic for manipulating the callback.

I don't think I'm far from having a working proposal, so we can decide then.

It has some similarities with Julia's dot notation for map and broadcast. Maybe some common ground can be found.
https://julialang.org/blog/2017/01/moredots/

[josdejong]

Sounds promising!

[dvd101x]

I found the issue that was affecting speed. Please check this on the new commit.

const a = math.random([1000])
const b = math.random([1000, 1])

console.time('add')
math.add(a, b)
console.timeEnd('add')            // 1003 ms

console.time('map typed')
math.map(a, b, math.addScalar)
console.timeEnd('map typed')      // 862 ms

console.time('map function')
math.map(a, b, (x, y) => math.addScalar(x, y))
console.timeEnd('map function')   // 897 ms

[josdejong]

Ow nice 😎

I get something like this:

// Chrome:
add: 741.507080078125 ms
map typed: 1483.096923828125 ms
map function: 638.359130859375 ms

// Firefox
add: 1996ms - timer ended
map typed: 2915ms - timer ended
map function: 2083ms - timer ended

// Node.js v20
add: 954.098ms
map typed: 2.316s
map function: 1.430s

Anyway: the performance of using multi map is close to not using it, which is really nice and better than I had expected!

[dvd101x]

Thanks Jos!

I included some tests and added some documentation in matrices.

I will try the next few days to include the type definitions.

[dvd101x]

Hi, it's ready for review.

[josdejong]

That sounds good, thanks for the updates! I'll look into it after the holidays (sorry for the delay).

[dvd101x]

Thank you Jos!
happy holidays!

[josdejong]

I've reviewed the latest version @dvd101x . It's really impressive how it looks now compared to your first draft: the code is now easy to follow, good performance, all is well documented and tested. Nice job!!

I added a few minor inline comments, can you have a look at those? After that we can merge this new feature :)

[dvd101x]

Hi Jos, thank you for the thorough review and your kind words.

It wouldn't be possible without your help and patience.

The comments are resolved now, except for one.

While looking into optimization of the multiple map feature I saw an opportunity to improve speed with applyCallback that maybe could improve forEach, filter and others. I would like to address this in a separate discussion if I find a way to make it work.

I see there is also similar objective in #3251 but I think it could still be applied after that.

[josdejong]

All looks good, merging now.

[josdejong]

Published now in v13.1.0, thanks again David 🎉

[dvd101x]

Thank you Jos!