Resolve NaN issue in Math with a few tests. #589

Cormac-Williams · 2016-11-21T15:10:26Z

Should resolve #539

olafleur · 2016-11-21T15:19:39Z

test/math.ts

+    insert: [],
+    remove: [],
+  };


I would suggest adding errors: true here to make sure that we effectively have an error.

olafleur · 2016-11-21T15:20:20Z

test/math.ts

+    insert: [],
+    remove: [],
+  };


olafleur · 2016-11-21T15:20:31Z

test/math.ts

+    insert: [],
+    remove: [],
+  };


Lol, I tried that and pushed too early. errors: true seems to cause the test to expect an error to be raised in some way from the code. I reverted, and may need to receive guidance from the team.

Filtering NaN is a constraint, that causes the block to filter any results rather than a specific error condition that needs to be raised.

Yeah, the behavior you have is correct here. What we want to verify is that if anything would otherwise return NaN that it instead returns nothing. It's not an error, it's the absence of a result.

olafleur · 2016-11-21T15:23:23Z

src/runtime/providers/math.ts

@@ -473,4 +483,4 @@ providers.provide("ln10", LN10);
 providers.provide("log2e",LOG2E );
 providers.provide("log10e",LOG10E );
 providers.provide("sqrt1/2", SQRT1_2);


What is sqrt1/2 ?

Square root of a half.

Right. As in i ! :)

No i is root -1 ;-)

Yeah. I am confused. You are right.

This reverts commit ece4643.

This reverts commit e031349.

ibdknox · 2016-11-21T19:08:37Z

src/runtime/providers/math.ts

-    proposal.providing = proposed;
-    proposal.cardinality = 1;
-    return proposal;
+    if(this.returns.length) {


Out of curiosity, why are you checking if there's a return?

To bring the code in line with the way that providers\logical.ts handles the same situation.
There's a slight discrepancy with the documentation in that it isn't returning a proposal with cardinality zero, but I've been unable to show that simply returning produces any strange behavious, so assumed returning an undefined is handled correctly by the runtime.

Ah the logical providers do this for a reason that doesn't really apply here. There are two cases for the logic operators, the standard filtering case x > y and the value returning case using is(x > y). In the former case, we never want to propose anything and instead just go through the testing phase. In the latter case, is adds a return variable that we'll want to fill in with the boolean value of the expression. With math ops, they should always have a bound return, so the check doesn't buy us anything.

Ok, I'll revert the getProposal function to original.

ibdknox · 2016-11-21T19:10:12Z

src/runtime/providers/math.ts

@@ -63,6 +68,11 @@ function degreesToRadians(degrees:number){
 }

 class Add extends TotalFunctionConstraint {
+  resolveProposal(proposal, prefix) {


Why override resolveProposal? Seems like add should have the same NaN guard.

At the moment the add function in the Math file is also providing the ability to concatenate strings to the language. If the decision is that Eve should always use string interpolation then this can be removed.

My opinion is that concatenating strings with a + symbol is such second nature to programmers that it would provide a real WTF moment to filter a block anytime "" + "" is used.

A fair number of languages have explicit concatenation operators instead of overloading + for strings. The main argument against doing so is that it doesn't make a lot of sense without implicit conversions which is something we want to stay far away from. E.g. "foo" + 1 implicit converts 1 to a string in JS. I think we should do more than just fail silently in the case where you try to add strings, but we need to think a bit about what that warning system would be.

At the moment you don't fail silently, you add them using Javascripts implicit rules :-) . I didn't want to introduce a breaking change to other peoples Eve code, but I agree that some thought needs to be given to implicit conversion. What about a combine aggregate?

I'll leave this as is for the pull unless told otherwise.

ibdknox · 2016-11-30T21:57:47Z

src/runtime/providers/math.ts

@@ -13,7 +13,9 @@ abstract class TotalFunctionConstraint extends Constraint {
  // proposed variable.
  resolveProposal(proposal, prefix) {
    let {args} = this.resolve(prefix);
-    return [this.getReturnValue(args)];
+    let result = this.getReturnValue(args);
+    if (isNaN(result) || !(isFinite(result))) {return [];}


Inifinity is a well-defined value, so I think we can remove that check and just have the NaN one.

Removed the check. This was also was what was handling the filtering for Divide by zero, so handled that in Div function.

…getProposal" This reverts commit ecf4231.

ibdknox · 2016-12-01T19:14:49Z

Looks great, we just need to fix the merge conflict in math.ts and in she goes :)

Cormac-Williams · 2016-12-01T19:54:34Z

Done :-)

ibdknox · 2016-12-01T20:02:27Z

Huh, looks like something went sideways on the merge - there are actually more conflicts now and @cmontella's look like they maybe got overwritten?

This reverts commit e75f60f.

Cormac-Williams · 2016-12-01T22:15:41Z

Ouch, well that's not as intended.
I'm going to revert the conflict merge.
Pull from master.
Apply changes again.
Hope and pray :-)

…math_NaN2" This reverts commit 15d7dbe, reversing changes made to 46e81a5.

Cormac-Williams · 2016-12-01T22:26:50Z

Ok, that went even more sideways. Sometimes I hate git :-(

I'm going to kill this pull request and send afresh with just the changes wanted. There'll soon be more commits than lines changed ;-)

cmontella · 2016-12-01T23:02:17Z

Thanks, sorry for the issues.

Cormac-Williams · 2016-12-01T23:04:00Z

Not your fault, my muckups.

Recreated as #629.

Resolve NaN issue in Math with a few tests.

963c241

olafleur reviewed Nov 21, 2016

View reviewed changes

Cormac-Williams added 4 commits November 21, 2016 15:34

Add a couple of errors: true statements.

e031349

Missed one errors: true

ece4643

Revert "Missed one errors: true"

c4e073e

This reverts commit ece4643.

Revert "Add a couple of errors: true statements."

ae4bda3

This reverts commit e031349.

ibdknox reviewed Nov 21, 2016

View reviewed changes

joshuafcole assigned joshuafcole and ibdknox and unassigned joshuafcole Nov 21, 2016

ibdknox reviewed Nov 30, 2016

View reviewed changes

Cormac-Williams added 3 commits November 30, 2016 23:19

Rem Infinity check Handle Div by 0 Revert getProposal

ecf4231

Grr, code missing. Revert "Rem Infinity check Handle Div by 0 Revert …

f02b520

…getProposal" This reverts commit ecf4231.

Try again.

6d93357

Resolve merge conflict.

e75f60f

Cormac-Williams added 2 commits December 1, 2016 21:39

Revert "Resolve merge conflict."

46e81a5

This reverts commit e75f60f.

Merge branch 'master' of https://github.com/witheve/Eve into math_NaN2

15d7dbe

Revert "Merge branch 'master' of https://github.com/witheve/Eve into …

0beaf90

…math_NaN2" This reverts commit 15d7dbe, reversing changes made to 46e81a5.

Cormac-Williams closed this Dec 1, 2016

Cormac-Williams deleted the math_NaN2 branch December 1, 2016 22:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve NaN issue in Math with a few tests. #589

Resolve NaN issue in Math with a few tests. #589

Cormac-Williams commented Nov 21, 2016

olafleur Nov 21, 2016

olafleur Nov 21, 2016

olafleur Nov 21, 2016

Cormac-Williams Nov 21, 2016

ibdknox Nov 21, 2016

olafleur Nov 21, 2016

olafleur Nov 21, 2016

Cormac-Williams Nov 21, 2016

olafleur Nov 21, 2016

Cormac-Williams Nov 21, 2016

olafleur Nov 21, 2016

ibdknox Nov 21, 2016

Cormac-Williams Nov 21, 2016

ibdknox Nov 30, 2016

Cormac-Williams Nov 30, 2016

Cormac-Williams Nov 30, 2016

ibdknox Nov 21, 2016

Cormac-Williams Nov 21, 2016

ibdknox Nov 30, 2016

Cormac-Williams Nov 30, 2016

ibdknox Nov 30, 2016

Cormac-Williams Nov 30, 2016

ibdknox commented Dec 1, 2016

Cormac-Williams commented Dec 1, 2016

ibdknox commented Dec 1, 2016

Cormac-Williams commented Dec 1, 2016

Cormac-Williams commented Dec 1, 2016

cmontella commented Dec 1, 2016

Cormac-Williams commented Dec 1, 2016

Resolve NaN issue in Math with a few tests. #589

Resolve NaN issue in Math with a few tests. #589

Conversation

Cormac-Williams commented Nov 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibdknox commented Dec 1, 2016

Cormac-Williams commented Dec 1, 2016

ibdknox commented Dec 1, 2016

Cormac-Williams commented Dec 1, 2016

Cormac-Williams commented Dec 1, 2016

cmontella commented Dec 1, 2016

Cormac-Williams commented Dec 1, 2016