operations between jagged arrays and scalars

[dvd101x]

Hi, this is the proof of concept discussed at #3537

It's more inline with what Jos and Glen suggested as my initial idea was flawed. This PR only shows two main changes.

• refactors matAlgo14xDs to use it's DenseMatrix map method and includes a hack to make the matrix creation faster.
• makes a new matAlgo15xAs to map arrays that can be jagged or non homogeneous and there is no need to convert to matrix and then back to array.

My expectation is that all operations between Matrix|Array and scalar should be faster.

The new functionality is that it's possible to do stuff like

math.add([1,[2,3]], 1)

math.config({matrix:"Array"})
math.evaluate("[1,[2,3]] + 1")

[josdejong]

Nice! This actually simplifies the code since you can "just" use the map function.

What are the next steps? Add some unit tests for jagged inputs, update docs, and see what the performance impact is?

[dvd101x]

Yes exactly, I will proceed with the next steps in the following weeks.

Maybe later I will come back to broadcasting jagged arrays.

[gwhitney]

Excellent, this is shaping up. A few more things to consider.

[dvd101x]

After including benchmarks, there is a big benefit of not converting arrays to matrices and small benefit of using the DenseMatrix.map method.

[dvd101x]

Hi, all comments are addressed either in code or as a reply. This is ready for review again.

[gwhitney]

Now we come to the difficulty that at this point, PRs are supposed to have per-function HISTORY for the functions they touch. I had hoped to get one of my PRs that touches a lot of functions to the point of going in to alleviate this big task before anyone else, but with the rate that Jos can review things and by other coincidences it seems as though you are getting there first. How would you like to handle this? Might we split up the chore of inserting such histories for the functions touched in this PR? Let me know your thoughts.

[dvd101x]

Most of the functions touched in this PR were due to the removal of concat.

I've been reviewing the history and this is mostly my fault as I did include them at #2895 because it was needed but then it should have been removed at #3220.

Please help me with an example of the history and I can gladly replicate it in the functions where concat was removed as a dependancy.

We could split the rest.

[gwhitney]

Please help me with an example of the history and I can gladly replicate it in the functions where concat was removed as a dependency.

A good current example is src/expressions/parse.js. Note that I do try to make the histories be from inception. The command git log --follow -- filename is super helpful for that as it tracks the files across renames as well -- some of the function implementations previously lived in different subdirectories, for example.

We could split the rest.

Yes, that would be absolutely fine. Please just let me know which functions are "assigned" to me and when is a good time for me to work on them that I can push a commit to this PR without interfering with your work. Thanks!

[gwhitney]

Wow, thanks for all of the updated history files. Let me know if there are others on my list to supply History for. I also see failed tests, but didn't look at the details. Are these something you'll be taking care of? Thanks again.

[dvd101x]

I think the history files are done. Just please validate the changes made.

The error was due to formatting, so it should be ok now.

Just a clarification, some of the reference history files are sorted from newest to oldest and some are from oldest to newest. I also didn't apply them consistently. So please let me know which way to go. As a reference in python documentation they are from oldest to newest.

I still have one comment to address.

[gwhitney]

So please let me know which way to go. As a reference in python documentation they are from oldest to newest.

Since these are now the bulk of the HISTORY entries in the whole system, and most of them were newest to oldest, I just changed them all (and changed the archetype src/expressions/parse.js also to have this order). There are probably still some lurking oldest to newest, but we can fix those when the files are touched. Doing final review, then will merge.

[gwhitney]

Oh, actually in the final review, I checked out the reference to #3537, and noticed this comment. So I added a test that I think corresponds to the expected/desired behavior for those cases. But it fails, and doesn't just produce a different value, it throws an error, and it's not even an error about not supporting that sort of ragged array. So I think it's a bug that should be addressed in this PR. @dvd101x do you want to take a look, or should I? And in either case, should mathjs support map([[], [2]], square) and produce map([[], [4]) or should this be an error or have some other value? Thanks!

[dvd101x]

Thanks @gwhitney, I'll take a look and fix the failing test. It seems to be an issue in optimize callback with typed functions because math.map([[], [2]], x => x**2) works as expected.

[dvd101x]

Hi, just to confirm that this behavior was introduced by #3256 as stated at #3567.

I will look into a possible fix and look for the expected behavior in other languages.

[gwhitney]

Right, #3567 noted this behavior but did not address it, because at the time jagged arrays were not "officially allowed" by mathjs. Since this PR for the first time explicitly supports such operations, it is a good opportunity to settle what this behavior should be and implement it. Note that the decision could be to throw an error in this case, but if so, it should be a "better" error than "Too many arguments in square," which seems to come out of nowhere. Thanks!

[dvd101x]

Hi. It must be ok now. Made a new function to find the first value and added some tests for optimizeCallback.