-
Notifications
You must be signed in to change notification settings - Fork 257
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
Refactor and add various new functions. #535
Conversation
Good job! It's a lot cleaner this way! Maybe it would be a good idea to add tests that demonstrate the behaviour of the new functions? |
return Math.cos(angle * (Math.PI / 180)); | ||
class ACosH extends ValueOnlyConstraint { | ||
acosh (x: number):number{ | ||
//How do we handle number outside of range in Eve? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Syntax error, I guess?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, more a feed back issue. Currently I'm returning NaN, but the language has as yet no formal type system, so in Eve programs NaN or Infinity come out as strings, which can be tested for. If there's ever a type system this can be cleaned up, but ACosH and ATanH functions only work for a certain range of numbers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I was not really clear. I meant : a possible solution would be to show a (red) error message in the Eve environment.
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can remove the blank lines from 219 to 221
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line not needed.
providers.provide("acosh", ACosH); | ||
providers.provide("atanh", ATanH); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can remove at least two lines here.
@@ -0,0 +1,273 @@ | |||
# Math Module |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the tests, I was thinking more of adding tests when you run npm test
(like in https://github.com/witheve/Eve/blob/master/test/join.ts for example)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lol, that wasn't deliberate, I'd added the file to my own repo and it automatically added itself when I pushed the clean lines commit.
Do you want to reject the eve file or should I wind the commits back?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can simply git revert 5fcba7ff
and push.
That should do the trick.
This reverts commit 5fcba7f.
Now you should be able to convert what you did in your Math_Test.eve file into node tests. Don't hesitate if you need help in the process ! :) |
Will do, just gotta learn how your test framework works now :-) |
To run the tests : |
"y": 1 | ||
} | ||
getReturnValue(args) { | ||
return [Math.atan2(args[0] ,args[1])]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you return an array. Shouldn't it just be a value?
return (y - 1 / y) / 2; | ||
} | ||
getReturnValue(args) { | ||
return [this.sinh(args[0])]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
return (y + 1 / y) / 2; | ||
} | ||
getReturnValue(args) { | ||
return [this.cosh(args[0])]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
return this.getAngle(angle); | ||
} else if (degrees !== undefined) { | ||
return Math.cos(degrees * (Math.PI / 180)); | ||
return [this.tanh(args[0])]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
} | ||
} | ||
getReturnValue(args) { | ||
return [this.asinh(args[0])]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
} | ||
|
||
getReturnValue(args) { | ||
return [this.acosh(args[0])]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
} | ||
|
||
getReturnValue(args) { | ||
return [this.atanh(args[0])]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
getReturnValue(args) { | ||
return Math.log(args[0])/Math.log(10); | ||
return [Math.exp(args[0])]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
|
||
class PI extends TotalFunctionConstraint { | ||
getReturnValue(args) { | ||
return [Math.PI]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And for all these constants I think?
Thanks Frank, that's what cut and paste gets you :-) |
Please enter the commit message for your changes. Lines starting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Builds fine. Most of these functions come from the JS library. Reviewed the hyperbolic trig functions and they look correct.
I'm going to merge this now after looking it over. The next thing to do here is fix all of our functions to return nothing instead of NaN, so I'll open an issue to track this: #539 |
As promised. Should also mention that there's a Math_Test.eve file in my examples folder that puts this stuff through its paces.
Full list of providers is now.
providers.provide("+", Add);
providers.provide("-", Subtract);
providers.provide("*", Multiply);
providers.provide("/", Divide);
providers.provide("log", Log);
providers.provide("exp", Exp);
//Trig and Inverse Trig
providers.provide("sin", Sin);
providers.provide("cos", Cos);
providers.provide("tan", Tan);
providers.provide("asin", ASin);
providers.provide("acos", ACos);
providers.provide("atan", ATan);
providers.provide("atan2", ATan2);
//Hyperbolic Functions.
providers.provide("sinh", SinH);
providers.provide("cosh", CosH);
providers.provide("tanh", TanH);
providers.provide("asinh", ASinH);
providers.provide("acosh", ACosH);
providers.provide("atanh", ATanH);
providers.provide("floor", Floor);
providers.provide("ceiling", Ceiling);
providers.provide("abs", Abs);
providers.provide("mod", Mod);
providers.provide("pow", Pow);
providers.provide("random", Random);
providers.provide("range", Range);
providers.provide("round", Round);
providers.provide("gaussian", Gaussian);
providers.provide("to-fixed", ToFixed);
//Constants
providers.provide("pi", PI);
providers.provide("e", E);
providers.provide("ln2", LN2);
providers.provide("ln10", LN10);
providers.provide("log2e",LOG2E );
providers.provide("log10e",LOG10E );
providers.provide("sqrt1/2", SQRT1_2);
providers.provide("sqrt2", SQRT2);