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

Diff function #1812

Closed
wants to merge 15 commits into from
Closed

Diff function #1812

wants to merge 15 commits into from

Conversation

Veeloxfire
Copy link
Contributor

Add function that returns difference between adjacent members of an array or matrix

Currently no way to decide which axis for multi-dimensional arrays but I could work on that

@Veeloxfire
Copy link
Contributor Author

That included a some old commits sorry

@Veeloxfire
Copy link
Contributor Author

#1634 and I forgot to link it

@josdejong
Copy link
Owner

Nice, thanks for your work @Veeloxfire .

I see the function diff currently supports 1-d matrices only. That is a nice and useful first version. I think we could go two ways:

  1. Built a check in to throw an error when the input is not a 1-d matrix.
  2. Go for the next step already: allow 2-d matrices too, and then allow selecting an axis along which to calculate the diff. This version would also require a check for the number of dimensions I think.

What would you like to do? Round it up or go for the next step right away?

@Veeloxfire
Copy link
Contributor Author

Ill start trying to get it to work for multi-dimensional so it more mirrors the numpy version as that was the original suggestion

@josdejong
Copy link
Owner

Sounds good 👍 . It will also be consistent with mathjs functions like sum, max, min, etc.

@Veeloxfire
Copy link
Contributor Author

Finally got around to doing this. The only thing its doesnt have that the numpy version does is the ability to say how many times to find the difference. But python has the ability to select parameters with the function(param=num) feature so it makes more sense to have optional parameters. I think to include that here would just be confusing and lead to hard to diagnose errors

@josdejong
Copy link
Owner

Cool! I'll do some testing this weekend.

],
examples: [
'diff([1, 2, 4, 7, 0])',
'diff([1, 2, 4, 7, 0], 0)',
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this example should be a 2d array so you can actually see what the dimension does, like

'diff([[1, 2], [3, 6]], 0)',
'diff([[1, 2], [3, 6]], 1)',

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I think I did this because it was just a copy and paste but yes should definitely have more comprehensive examples

*
* const arr = [[1, 2, 3, 4, 5], [1, 2, 3, 4, 5], [9, 8, 7, 6, 4]]
* math.diff(arr) // returns [[0, 0, 0, 0, 0], [8, 6, 4, 2, -1]]
* math.diff(arr) // returns [[1, 1, 1, 1], [1, 1, 1, 1], [-1, -1, -1, -2]]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think here the dimension should be specified, like math.diff(arr, 1)?

@josdejong
Copy link
Owner

Looks very good @Veeloxfire 👍

I made two small remarks in the code, and here three more remarks:

  1. Can you add some unit tests testing with BigNumber, Fraction, Unit? I tested the following it and it all works:

    assert.deepStrictEqual(math.diff([[1, 2], [3, 6]]), [[2, 4]])
    assert.deepStrictEqual(math.diff([[1, 2], [3, 6]], 0), [[2, 4]])
    assert.deepStrictEqual(math.diff([[1, 2], [3, 6]], 1), [[1], [3]])
    
    assert.deepStrictEqual(math.diff(math.bignumber([[1, 2], [3, 6]])), math.bignumber([[2, 4]]))
    assert.deepStrictEqual(math.diff(math.bignumber([[1, 2], [3, 6]]), 0), math.bignumber([[2, 4]]))
    assert.deepStrictEqual(math.diff(math.bignumber([[1, 2], [3, 6]]), 1), math.bignumber([[1], [3]]))
    
    assert.deepStrictEqual(math.diff(math.fraction([[1, 2], [3, 6]])), math.fraction([[2, 4]]))
    assert.deepStrictEqual(math.diff(math.fraction([[1, 2], [3, 6]]), 0), math.fraction([[2, 4]]))
    assert.deepStrictEqual(math.diff(math.fraction([[1, 2], [3, 6]]), 1), math.fraction([[1], [3]]))
    
    assert.deepStrictEqual(math.diff([math.unit('2 cm'), math.unit('8 cm')]), [math.unit('6 cm')])
  2. Can you add a unit test testing that an error is thrown when you pass and invalid dimension (like 999) or invalid input argument (like not passing an array but a value or something)

  3. The expression parser of mathjs is 1-based instead of 0-based, see docs. This means that we have to write a transform function for diff so that when using it inside the expression parser, you can pass 1-based dimensions instead of 0-based. Functions like sum also have such a transform. See:

    https://github.com/josdejong/mathjs/tree/develop/src/expression/transform
    https://github.com/josdejong/mathjs/blob/develop/src/expression/transform/sum.transform.js

    If you're not sure what/how to do this I can implement the transform function for diff and a unit test for it, no problem at all.

@josdejong
Copy link
Owner

@Veeloxfire would be great if you can have a look at my feedback

@Veeloxfire
Copy link
Contributor Author

Sorry I didnt see this! Yep Ill look in to it

@Veeloxfire
Copy link
Contributor Author

Veeloxfire commented Jul 9, 2020

I will have to make another pull request to update this because I decided to reclone the repository to refresh my offline one but I stupidly deleted the old one

Ill make sure to reference this one when I do

@josdejong
Copy link
Owner

👍 thanks

@Veeloxfire
Copy link
Contributor Author

I'll close this pull request now as its moved to #1920

@Veeloxfire Veeloxfire closed this Jul 17, 2020
@josdejong
Copy link
Owner

👍 I'll look into your new PR soon

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

Successfully merging this pull request may close these issues.

None yet

2 participants