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

Add bigdecimal capability #32

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

gbenroscience
Copy link
Owner

@gbenroscience gbenroscience commented Feb 13, 2023

This PR exposes bigdecimal arithmetic features to ParserNG users. This is the first stage of this feature.

For normal floating point arithmetic, we use the MathExpression class.
For bigdecimal arithmetic, the active class is the BigMathExpression class.

What is supported

  1. BigDecimal arithmetic operations on numbers and variables
  2. BigDecimal arithmetic operations on functions that are composed only of numbers and variables and other such functions that can be broken down to numbers and variables alone.

What is not supported yet

  1. BigDecimal arithmetic operations on inbuilt functions
  2. BigDecimal arithmetic operations on user defined functions that include inbuilt functions
  3. Logical expressions are not enabled yet(feature disabled), in contrast to the MathExpression class which handles boolean expressions easily.

Still to come

  1. Further updates as more functions that support bigdecimal arithmetic are contributed

So this will work:

        BigMathExpression bme = new BigMathExpression("x=2;h(x)=3*x^2+x+2*x;h(3);");
        System.out.println(bme.solve());

But this wont:

        BigMathExpression bme = new BigMathExpression("x=2;h(x)=3*x^2+x+2*x-sin(x);h(3);");
        System.out.println(bme.solve());

The last expression, h(3) will fail at runtime because it contains sin(x) (an inbuilt function) in its definition, whose BigDecimal implementation is non-existent.

"Will fail at runtime" implies that the function: h(x)=3*x^2+x+2*x-sin(x) will be successfully compiled and stored in parser memory, but when we try to use it by running h(3), it will give an error

@gbenroscience
Copy link
Owner Author

@judovana. Please check

String val = expr.solve();
String referenceName = expr.getReturnObjectName();
System.out.println("rhs: "+rhs+", mathExpClass: "+mathExpClass+", expr.class: "+expr.getClass()+", val: "+val+", type: "+expr.getReturnType());

Choose a reason for hiding this comment

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

Suggested change
System.out.println("rhs: "+rhs+", mathExpClass: "+mathExpClass+", expr.class: "+expr.getClass()+", val: "+val+", type: "+expr.getReturnType());

Copy link
Contributor

Choose a reason for hiding this comment

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

Yah, tracing/verbose mode is a bit missing.

@judovana
Copy link
Contributor

Ugh. This is very hard to read. It seems to be mixing various reformatting and refactoring together with logical changes. I guess youa re not willing to split those to individual commits?
Permanent usage of main methods as testing area with various commented out/in features - unittests are here fort his.
Generally speaking thei feature lack unittests at all - mainly it lacks targetted unitteest for the touched coede.
Missing unittests for new logic.
Various irrelevant outputs to stdout. If yuu really need the,m the verbose mode should be added. While it is not properly here, they should be printed out to stderr
The usage of return true;//contraband is very error prone, should ber hadled by BigMathExpression design, or exception should be thrown if it is not honoured.
The main problem remains the huge duplcicated code, mainly in BigMathExpression . Such thing bites.

If you would be my intern all above will be show stoppers for merging

  • do the refactoring in seaprate commit (maybe refactor whole parser ng in one huge comment?)
  • do the cosmetic changes (eg changes in comments) in separate commit
    • until this is in, no more reviews
  • implement logging/debuggin/tracing logic
  • add unittests instead of main methods
  • add unittests targetting the feature
  • although it can maybe go in with duplcated code, have a plan how to get ird of it before push
    • only having the plan may be enough for not letting the duplicated code in, because the plan to get rid of it will be good

But you are not my intern, and this is yours project :) So only good advices can be given.

One political request if I can. I have PR adding docs for expanding and logical parsers and bumpiong ParserNG to 0.1.9. As this PR may be possibly breaking change, do you mind to release 1.9 before continuing with feature ?

@gbenroscience
Copy link
Owner Author

gbenroscience commented Feb 13, 2023

Ugh. This is very hard to read. It seems to be mixing various reformatting and refactoring together with logical changes. I guess youa re not willing to split those to individual commits? Permanent usage of main methods as testing area with various commented out/in features - unittests are here fort his. Generally speaking thei feature lack unittests at all - mainly it lacks targetted unitteest for the touched coede. Missing unittests for new logic. Various irrelevant outputs to stdout. If yuu really need the,m the verbose mode should be added. While it is not properly here, they should be printed out to stderr The usage of return true;//contraband is very error prone, should ber hadled by BigMathExpression design, or exception should be thrown if it is not honoured. The main problem remains the huge duplcicated code, mainly in BigMathExpression . Such thing bites.

If you would be my intern all above will be show stoppers for merging

  • do the refactoring in seaprate commit (maybe refactor whole parser ng in one huge comment?)

  • do the cosmetic changes (eg changes in comments) in separate commit

    • until this is in, no more reviews
  • implement logging/debuggin/tracing logic

  • add unittests instead of main methods

  • add unittests targetting the feature

  • although it can maybe go in with duplcated code, have a plan how to get ird of it before push

    • only having the plan may be enough for not letting the duplicated code in, because the plan to get rid of it will be good

But you are not my intern, and this is yours project :) So only good advices can be given.

One political request if I can. I have PR adding docs for expanding and logical parsers and bumping ParserNG to 0.1.9. As this PR may be possibly breaking change, do you mind to release 1.9 before continuing with feature ?

lol, sorry.. I created ParserNG in 2009. So well, a lot of design decisions taken then would not fly now. I was more concerned about creating the parser and making it multi-featured then.

@gbenroscience
Copy link
Owner Author

One political request if I can. I have PR adding docs for expanding and logical parsers and bumpiong ParserNG to 0.1.9. As this PR may be possibly breaking change, do you mind to release 1.9 before continuing with feature ?

Send in the PR

@gbenroscience
Copy link
Owner Author

gbenroscience commented Feb 13, 2023

The usage of return true;//contraband is very error prone, should ber hadled by BigMathExpression design, or exception should be thrown if it is not honoured.

The method is marked private and the exception is thrown in the BigMathExpression constructor or am I missing something?

@gbenroscience
Copy link
Owner Author

Permanent usage of main methods as testing area with various commented out/in features - unittests are here fort his.
Generally speaking thei feature lack unittests at all - mainly it lacks targetted unitteest for the touched coede.
Missing unittests for new logic.

True. Will add unit tests where relevant, going forward. Thanks

@judovana
Copy link
Contributor

judovana commented Feb 13, 2023

The usage of return true;//contraband is very error prone, should ber hadled by BigMathExpression design, or exception should be thrown if it is not honoured.

The method is marked private and the exception is thrown in the BigMathExpression constructor or am I missing something?

I guess no. More likely I may be missing something. Can you point up a line please>

@judovana
Copy link
Contributor

lol, sorry.. I created ParserNG in 2009. So well, a lot of design decisions taken then would not fly now. I was more concerned about creating the parser and making it multi-featured then.

No need to apologise. Still parseng is one of the younger. Also it is indeed feature-full. Still the tests are now there, and should be used right away O:)

ps: mixing refactroign reformating and logical changes is one of the most terrible evils in sowftare development

Still, thanx a lot for working on this!

@judovana
Copy link
Contributor

I would still heavil vote for heavy rework of initialcommits - to split refactorings, cosemtic changes, a and reallogic change.
If this is done (then the logic change will be reviwable) then soem effort how to remove duplicated code shoudl be done ( I will be happy to help)

Tahnx!

@judovana
Copy link
Contributor

Jsut for small info - it wil be easy to adapt main CLI to use this new BIgDecimal parser. Also all "underlying" parsers (like expandable and logical) are already parametrised so they can work with BigDecimalMath out of the box.
Great work. TYVM!

@judovana
Copy link
Contributor

Hello!

I'm running set of JMH benchmarks - basic double x Double x BigDecimal toString/fromString and arithemtical operations. and advanced, with ParserNG MathExpression double x BigMathExpression BigDecimal implemenataion and also on a bit rush DoubleMatheExpression on Double.

It is currently moreover showing that double Double is practically no difference. Which should lead this PR to nearly complete removal double, and making MathExpression easily extendable as BigDecimal, Float, or generally any Number.

I hope to have some publishable results to the end of next week.

In meantime... Maybe 0.1.9 can be released.
In meantime... all the refactorings and cosmetic changes can be pushed, so this PR will be clear and clean?

@judovana
Copy link
Contributor

judovana commented Feb 18, 2023

Ok, for now, just to String and from string:

https://github.com/judovana/CustomJmhBenchmarks/tree/numbers

It is interesting. It seems that it should move directly to BigDecimal :)

float < BigDecimal << double

and

float == Float and double == Double

So in the autoboxing, java is doing really great work. I am looking forward to arithmetic measurements and to ParserNG itself. Not sure when I will have more time :(

@judovana
Copy link
Contributor

judovana commented Feb 19, 2023

now some basic math:

https://github.com/judovana/CustomJmhBenchmarks/blob/numbers/README.md#basic-math

looks more like expected . More autoboxing in equation, the worse. However it is still very minor, and generally double=Double=float=Float

The BigDecimal however, is here super slow (which is logical, as it do not count in cpu but algorithmically)

@judovana
Copy link
Contributor

judovana commented Feb 19, 2023

MathExpression x BigMathExpression:

https://github.com/judovana/CustomJmhBenchmarks/blob/numbers/README.md#mathexpression-x-bigmathexpression

In the complexity of parser, the slowdown of the move from double to BigDecimal is nealry nothing.

I'm planning to add also include DoubleMathExpression - jsut for this meassurments - but from all we see, the impact of double x Double is nothing. Impact of double x BigDecimal is nearly nothing.

Thus saying, the work should be done to make MathExpression properly extend-able, And thus have no duplicated code nowhere and support in functions out of the box.

@judovana
Copy link
Contributor

judovana commented Feb 20, 2023

Originally I wanted to write some DoubleMathExpression as you did in BigMathExpression, but then I chaged my mind and simply converted whole ParserNG from double to Double -judovana@352745f

Results are here: https://github.com/judovana/CustomJmhBenchmarks/blob/numbers/README.md#parserng-rewritten-from-double-to-double

interesting is, that whole ParserNG got nearly immeasurable, but still visible speed up.

That again is saying, that adding BigMathExpression should be done by heavy changes elsewhere, and BigMathParser should then be just candy on top of it all.

Note, that without support of functions, BigMathParser is useless, and should not go in. Functions are heartbeat of ParserNG. Thus saying, I'm against this PR (but I do not have any vote).
At least the functions should be enabled as in fallback mode, so they do nto provide necessary precission, but are accessible.

As input to functions is String - and I consider it as excelent - a converting factory must be somewhere provided. And as Number interface do not enforce mult/div/add/subs function ... well, its hard task.

Unless you will require additional measurement, I think I'm finished with them.

@judovana
Copy link
Contributor

Oh, one important nit. With the BigMathExpression parser comes need of globally controlled MathContext. See https://github.com/gbenroscience/ParserNG/blob/master/src/main/java/math/BigDecimalNthRootCalculation.java#L12

I already did it wrong, as I was really not forseeing BigMathExpression - and eg here: https://github.com/gbenroscience/ParserNG/blob/master/src/main/java/parser/methods/ext/Avg.java#L22 it is redeclared.

@gbenroscience
Copy link
Owner Author

Originally I wanted to write some DoubleMathExpression as you did in BigMathExpression, but then I chaged my mind and simply converted whole ParserNG from double to Double -judovana@352745f

Results are here: https://github.com/judovana/CustomJmhBenchmarks/blob/numbers/README.md#parserng-rewritten-from-double-to-double

interesting is, that whole ParserNG got nearly immeasurable, but still visible speed up.

That again is saying, that adding BigMathExpression should be done by heavy changes elsewhere, and BigMathParser should then be just candy on top of it all.

Note, that without support of functions, BigMathParser is useless, and should not go in. Functions are heartbeat of ParserNG. Thus saying, I'm against this PR (but I do not have any vote). At least the functions should be enabled as in fallback mode, so they do nto provide necessary precission, but are accessible.

As input to functions is String - and I consider it as excelent - a converting factory must be somewhere provided. And as Number interface do not enforce mult/div/add/subs function ... well, its hard task.

Unless you will require additional measurement, I think I'm finished with them.

Thanks for all the work @judovana
I apologize for being away for so long. Will do my best to find time to look at it all again, and also release 0.1.9

@judovana
Copy link
Contributor

judovana commented Mar 5, 2023

no worries sir :)

I believe the only missing piece is some class NumberProvider, which will be converting strings and numbers, and returning some Number on demand as needed. Then MathParser, FloatMathParser and BigMathParser, built on top of some GenericNumberProvidingParser should be easy enough.

As functions are taking array of Strings, they will need to handle on theirs own and support, or not support double/big/float inputs. Or the contract of functions will need to be changed, so they would receive already parsed numbers, and then the support will be just in overloading of some solve(<double/float/big> array) . However still many of implementations will need to be type-dependent similarly as the basic math operations above big x float/double

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

3 participants