support for decimal about jace

[mesika]

Thanks @pieterderycke I appreciate it.
I need the decimal to support money calculations. double do not provide a good accuracy, and, depend on the calculations might yield less accurate results.

[JulianRooze]

@rcl2748 the need for Jace in our application got put on the backburner, so never got around to creating a proper pull request for this, but if creating a custom build is an option for you (rather than waiting for an updated NuGet package) then my branch contains fully working code for decimal support:

https://github.com/JulianRooze/Jace/tree/feature/decimal-support

Just clone and build and you should be good to go.

[pieterderycke]

It is possible, but it has quite some impact.
It is something that is already on my personal todo for sometime. But I did not yet found the time to start working on this.

[JulianRooze]

As I would like to use Jace for calculations involving money, decimal support is really important for me as well. I've begun with a really naive port of Jace where I simply cloned the project and started replacing "double" with "decimal" whenever I ran into it, but you quickly run into the code that emits the IL and there is no trivial way to replace the IL for doubles with IL for decimals because double is supported natively in IL while decimal is not so the IL is much more complicated (just emitting a constant is much harder). Maybe it's because I'm not very familiar with IL emission, but the easiest solution for me to this would be to use the .NET Expression API, which does natively support decimal, and compile an expression to a dynamic method.

I also don't know if it's feasible to make this change while also keeping double support. To me it seems like that wouldn't be really possible without introducing a secondary DecimalCalculationEngine API for decimal. But that would at least allow you to reuse the existing parsing/AST building code.

EDIT: I see the DynamicCompiler already uses the Expression API when compiled against NETFX_CORE.

[pieterderycke]

I will consider the support of adding decimal. But I need to investigate a clean way to offer both double and decimal support through the API. If you have an idea, please let me know. It is not an issue to support the necessary IL code. So don't worry about that aspect :-)

[JulianRooze]

Well, it might be nice to do something with generics. The public API of CalculationEngine can easily be made generic:

public interface ICalculationEngine<T>
{
    void AddConstant(string constantName, T value);
    void AddFunction(string functionName, Func<T, T, T, T, T, T, T> function);
    void AddFunction(string functionName, Func<T, T, T, T, T, T> function);
    void AddFunction(string functionName, Func<T, T, T, T, T> function);
    void AddFunction(string functionName, Func<T, T, T, T> function);
    void AddFunction(string functionName, Func<T, T, T> function);
    void AddFunction(string functionName, Func<T, T> function);
    void AddFunction(string functionName, Func<T> function);
    Func<System.Collections.Generic.Dictionary<string, T>, T> Build(string formulaText);
    T Calculate(string formulaText);
    T Calculate(string formulaText, System.Collections.Generic.IDictionary<string, T> variables);
    Jace.Execution.FormulaBuilder Formula(string formulaText);
}

Of course, for backwards compatibility, you still want CalculationEngine to refer to the one that takes and returns doubles. Naming the decimal one the DecimalCalculationEngine is okay, but then maybe introduce some kind of other entry point that let's you say "I want a calculation engine for double/decimal". Something like:

public static class CalculationEngineFactory
{
    public static ICalculationEngine<T> Create<T>()
    {
        ..
    }
}

[JulianRooze]

Just a quick update on this. I went ahead and tried to make Jace work with both double as well as decimal. This is by no means a "please merge this" yet (it's a pull request for my own fork anyway ;)), more of a proof of concept.

JulianRooze#1

For the most part, it was a matter of turning classes that depended on double into classes with generics. For the code generation I've only implemented the code gen via de Expressions API, not through IL emit.

Using the decimal support now involves creating a DecimalCalculationEngine which is perhaps not that elegant. The CalculationEngine could possibly implement both ICalculationEngine<double> as well as ICalculationEngine<decimal> but there would be a hiding/clashing implementation of these two interface methods:

Jace.Execution.FormulaBuilder<T> Formula(string formulaText);
T Calculate(string formulaText);

Since they would differ only by return type, which wouldn't work.

For our own internal purposes for Jace, my changes are 'good enough' but I'd be happy to work on making it mergeable back to master.

There's probably some unnecessary code duplication still, especially the decimal interpreter is just a copy-paste of the double interpreter. I'll see if I can unify these again. Because there's no generics support for operators this would require adding an abstraction for additional/multiplication/etc, marginally slowing down the interpreter.

[pieterderycke]

Thanks for the suggested modfications. I will have a more detailed look at your code this weekend. I like the idea, I only don't like the code duplication... ;-)

[JulianRooze]

Agreed, so I just updated the code ;)

I got rid of the duplication with the interpreter by doing what I said in my last comment.

Also, I added some comments to the pull request to clarify/justify some of the code I've changed.

My eye also caught this other issue that was opened:

#13

It's probably pretty trivial to add BigInteger support to Jace now. The only real hurdle I see is the IL emit support. I don't want to dictate how you write your code of course, but isn't the Expression API pretty widely available on platforms now and couldn't you make it the default instead of IL emit and use that as a fallback only? Just an idea :)

[JulianRooze]

Have you had a chance to look over my suggested changes yet? :)

Like I said, for us the changes I've made work perfectly fine and are 'good enough', but of course it'd be nice if they could some day be contributed back to master.

Additionally, in the meantime I've mostly implemented BigInteger support as a proof of concept in very few lines of code and in a single file and it seems to work fine (aside from parts of the expression generator that rely on Math which of course has no overloads for BigInteger, so I need to add an abstraction there still). I've created a new project within the solution for it, as it adds a reliance on the System.Numerics assembly and it'd be a separate NuGet package as well.

Again, that's leaving aside the IL generation bits, which are just too scary for me ;)

[pieterderycke]

Sorry for the (very) late response :-)

We should pull your changes back in the master of Jace. Support for decimal and BigInteger would be an interesting feature for Jace.

And don't be afraid of the IL generation. I think we could better drop it. It is only required for older platforms such as Windows Phone 7. And support for these platforms can be easily dropped. So we can simplify the source code of Jace by doing that.

[JulianRooze]

Only just noticed your response! Sorry for my late response now ;)

That'd be great! I'll try to make my code ready for a pull request in the next few days. The BigInteger code isn't ready yet, but the decimal code is. I'll try to add tests for it first though.

[rcl2748]

Can you please add this feature. We are still waiting for it :)

[mesika]

Hi, any progress on decimal support? I've tried using @JulianRooze branch, but unfortunately some tests are failing on this branch.
For example: CalculationEngineTests -> TestFormulaBuilder():

[TestMethod]
public void TestFormulaBuilder()
{
    DecimalCalculationEngine engine = new DecimalCalculationEngine();
    Func<int, decimal, decimal> function = (Func<int, decimal, decimal>)engine.Formula("var1+2*(3*age)")
        .Parameter("var1", DataType.Integer)
        .Parameter("age", DataType.FloatingPoint)
        .Result(DataType.FloatingPoint)
        .Build();

    decimal result = function(2, 4m);
    Assert.AreEqual(26.0m, result);
}

With test result:

Message: Assert.AreEqual failed. Expected:<26.0>. Actual:<35936165692946818613102097204>.

[pieterderycke]

Hello @mesika I could look to bring proper support to the official version of Jace. But it was never a high priority for me, because double was good-enough for me. In which context would you require decimal support?

[RredCat]

Hi @mesika ,
I am also interested in support decimal for money calculation.
Looks like that NCalc supports it
Be careful, I haven't tested it yet.

Anyway, thank you @pieterderycke for your library.

[mrxrsd]

Hi!

Like Julian I need decimal support. So I've just updated Julian's solution of decimal's support.

All doubles tests are passing but need to create some tests for decimals calculations too.

https://github.com/mrxrsd/Jace/tree/feature/decimal-support-updated

---

I forgot to updated TokenReader, decimals version is not working yet.

[mrxrsd]

100% passed in all tests (double and decimal).

I've replicated all double's tests with the new decimal engine.

[mikalemason]

@mrxrsd thanks for the work on supporting Decimal - we'd really like to have this vetted and moved to the main build of Jace. Seeing as how it was originally called out as going to be supported with 1.0, is there any ETA on a proper merge and release with decimal support? @pieterderycke

[mrxrsd]

@mrxrsd thanks for the work on supporting Decimal - we'd really like to have this vetted and moved to the main build of Jace. Seeing as how it was originally called out as going to be supported with 1.0, is there any ETA on a proper merge and release with decimal support? @pieterderycke

You can check my repository: https://github.com/mrxrsd/jace

This feature and others fixes are merged there but I never publish a nuget package by my own.