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

Fractional values are defaulting to Decimal data type #22

Closed
DigitalPigeon opened this issue May 18, 2018 · 7 comments
Closed

Fractional values are defaulting to Decimal data type #22

DigitalPigeon opened this issue May 18, 2018 · 7 comments
Assignees
Labels
Milestone

Comments

@DigitalPigeon
Copy link

It seems that the built in functions all return Decimals?

So when I run a formula like this
([myFirstDouble]/([mySecondDouble]*9.8))*3.14*POW([myThirdDouble],2)

I will always get this error:
Operator '*' can't be applied to operands of types 'double' and 'decimal'

Does this mean that expressive is essentially not compatible with doubles as inputs when you use both functions and multiplication?

This might be related to #8

@DigitalPigeon
Copy link
Author

Actually, after more investigation, the issue are the constants.

The constants all seem to be decimals.

So these work
[myDouble]*5
[myDouble]*pow(5.5,2)
[myDouble]+5.5

But this will not work
[myDouble]*5.5

in fact, this doesn't even work
pow(5.5,2)*5.5

Am I missing something really simply to prevent fractional constants from being decimals and messing everything up?

@DigitalPigeon DigitalPigeon changed the title Is expressive not compatible with doubles? Fractional values are defaulting to Decimal data type May 18, 2018
@bijington
Copy link
Owner

Hi @DigitalPigeon,

Thanks for the bug report, you have found a certain issue. I have created the following unit test that confirms the issue you are seeing.

[TestMethod]
public void ShouldHandleBugTwentyTwo()
{
    // Not working
    //[myDouble]*5
    //[myDouble]* pow(5.5,2)
    //[myDouble]+5.5
        
    var expression = new Expression("[myDouble]*5");

    var arguments = new Dictionary<string, object>
    {
        ["myDouble"] = 4.0
    };

    Assert.AreEqual(20, (double)expression.Evaluate(arguments));

    expression = new Expression("[myDouble]*Pow(5.5,2)");

    Assert.AreEqual(121, (double)expression.Evaluate(arguments));

    expression = new Expression("[myDouble]+5.5");

    Assert.AreEqual(9.5, (double)expression.Evaluate(arguments));

    // Not working
    //[myDouble]*5.5
    expression = new Expression("[myDouble]*5.5");

    Assert.AreEqual(20, (double)expression.Evaluate(arguments));
}

To hopefully explain the cause of the issue, currently when parsing a number with a decimal point Expressive assumes that the decimal type is the best to use for accuracy. Currently there isn't a way of specifying the decimal data type, it is something on the list however I haven't been able to find a suitable solution so far. Always happy for ideas if anyone has them :).

Furthermore there is an issue where the code actually goes about performing the multiplication. It should convert the two participants to a common type and perform the multiplication. The code was originally written to match the .NET world so if you try multiplying var result = 5.5d * 5.5M; you will see a very similar error to the one that is thrown by Expressive. It does however seem that I have started but not finished performing these conversions. If you have access to the code you should see that in Numbers line 678 the conversion is done if the decimal is the left hand side. What needs to be done is also convert on line 661. It also needs to be rolled out for other operators too.

Unfortunately I am not quite in a position to publish a new version to fix this in the short term as I will be away until the start of June. When I am back I plan to do a release with support .NET Standard and I can wrap up this fix with it.

A short term workaround can be to make sure that the constant appears on the left hand side of the multiplication. So where [myDouble]*5.5 is not working this 5.5*[myDouble] currently does work.

Sorry for the lengthy for response but I hope it helps.

Shaun

@DigitalPigeon
Copy link
Author

Okay - After downloading the code, the "culprit" seems to be in ExpressionParser.cs line 346.

When you are detecting the data type of a constant, your order of preference is
int > decimal > double > float > long

So in fact almost all fractional constants will be decimals.

I think my only option here is to change the Numbers.cs file to support operations between Decimal and other types.

I won't bother sending you a PR with the change, as you mentioned in #8 that you want to keep the behavior matching .net - but without casting or converting, it really limits usability for me.

@DigitalPigeon
Copy link
Author

Shaun,
Sorry - I started my last reply before you had posted, so I hadn't seen your response.

Yup, seems like the same conclusion. And I did see your initial Convert.ToDecimal() changes in the Numbers.cs file.

I've gone through and changed the rest, and it seems to be working for me now in my particular test cases.

Thanks for looking at this so quickly! Wow! 👍

@bijington
Copy link
Owner

bijington commented May 18, 2018

@DigitalPigeon it is good to hear that those changes are holding up well enough for now. I am planning on making a release in mid June that will incorporate the fixes for you but do feel free to send in the PR if you want.

No problem on the response you managed to catch me at a good time :).

I do wonder if #20 will also allow the explicit entry of a double a constant inside an expression e.g. double(5.5)*[myDouble] although it starts to make the expressions look rather more bloated/complicated and would require more knowledge for a person creating these expressions which would be nice to avoid.

@bijington bijington self-assigned this Jun 7, 2018
@bijington bijington added the bug label Jun 7, 2018
@bijington bijington added this to the Next release milestone Jun 7, 2018
bijington added a commit that referenced this issue Jun 7, 2018
…type to correct division of double / decimal, etc.
@bijington
Copy link
Owner

@DigitalPigeon sorry I didn't give you an update to inform you that the latest release should resolve your issue. I hope it does.

@DigitalPigeon
Copy link
Author

You are a rockstar @bijington . Thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants