Skip to content

set operations#756

Closed
Nekomajin42 wants to merge 1 commit into
josdejong:developfrom
Nekomajin42:set_ops
Closed

set operations#756
Nekomajin42 wants to merge 1 commit into
josdejong:developfrom
Nekomajin42:set_ops

Conversation

@Nekomajin42

@Nekomajin42 Nekomajin42 commented Nov 27, 2016

Copy link
Copy Markdown
Contributor

Hey Jos!

It should work now. Can you delete my previous #703 pr?

@josdejong

Copy link
Copy Markdown
Owner

Thanks for your new PR @Nekomajin42 , it looks neat. I will review it this week (it's quite a large PR).

@josdejong

Copy link
Copy Markdown
Owner

Nice work Tóth, thanks again. Your code is very well readable, nice job.

I have two comments:

  • Can you add some unit tests to check whether Matrix input also results in Matrix output? Looking at the code it will, but a small unit test will let us know for sure and prevent any future regressions.
  • So far the new functions work for numbers but not BigNumbers and Fractions, and the functions give wrong results in that case. I think it will be quite easy to add support for BigNumbers and Fractions: you have to replace equality/inequality checks on the array values like b1[i].value === b2[j].value with math.equal(b1[i].value, b2[j].value) everywhere (and add some unit tests for them). Would be great if you could do that!

@Nekomajin42

Copy link
Copy Markdown
Contributor Author

I will do that.

Btw, my first name is Róbert. :)

@josdejong

Copy link
Copy Markdown
Owner

Ah, sorry Róbert! Thanks, looking forward to the updates.

@Nekomajin42

Nekomajin42 commented Feb 1, 2017

Copy link
Copy Markdown
Contributor Author

Hi Jos!

I am working on the changes you've asked. It seems to work for fractions and bignumbers, but if I have a complex number in an array, and try to create its union with another array, I get the following error: TypeError: No ordering relation is defined for complex numbers

Any idea?

EDIT:
How should I test the equality for fractions and bignumbers? The array/matrix output test works fine already.

@josdejong

Copy link
Copy Markdown
Owner

It's indeed not possible to order complex numbers since they have no ordering defined. You can compare whether two complex numbers are equal or not, but not if one is larger than the other. The only way to solve this is to have the set functions not rely on sorted arrays but only use equal checks. Would that be possible?

@Nekomajin42

Copy link
Copy Markdown
Contributor Author

I think it will be easy. I wanted to output a sorted result, but it is not necessary. I will give it a try.

@josdejong

Copy link
Copy Markdown
Owner

👍

@Nekomajin42

Copy link
Copy Markdown
Contributor Author

I guess I should use equal in utils/array.js for my identify funtion. How should I call it?
var equal = require('../function/relational/equal'); is not working. Throws an error in the tests.

@josdejong

Copy link
Copy Markdown
Owner

I don't know what error you get, so I'm not sure, but it's probably because the utils functions only contain low level JS stuff (working with plain numbers, objects, arrays). They don't have access to the context of math.js with these high level functions like function/relational/equal.js, which contain a factory function which must be called via load(require(...)).

To solve this you should move your identify function out of utils/array.js into a new file named lib/function/set/identify.js, and put it inside a factory function like all of the new set functions. Then it can load equal similar to how you load subset for example:

function factory (type, config, load, typed) {
  var equal = load(require('../relational/equal'));
  // ...

  return function identify (...) {
    // ...
  }
}

// we don't export exports.name because we don't want to expose 
// the identify function in `math` 
exports.factory = factory;

@Nekomajin42

Copy link
Copy Markdown
Contributor Author

Well, I left the == check for now in the helper function, and it seems to work fine. I will do a pr soon.

@josdejong

Copy link
Copy Markdown
Owner

Using == works correct if the objects (like a BigNumber) have a string representation is the same, like:

math.bignumber(2) == 2 // true

that may be the exact behavior we want, what do you think? I will await your PR and give pay some extra extension to this.

@Nekomajin42

Copy link
Copy Markdown
Contributor Author

I think you are correct. I will try to send the pr this week to finish this issue finally.

@Nekomajin42 Nekomajin42 mentioned this pull request May 21, 2017
@josdejong

Copy link
Copy Markdown
Owner

@Nekomajin42 we can close this PR too right?

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.

2 participants