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

Suppress conversion into e function #61

Closed
S-Man42 opened this issue Jan 17, 2022 · 18 comments · Fixed by #89
Closed

Suppress conversion into e function #61

S-Man42 opened this issue Jan 17, 2022 · 18 comments · Fixed by #89
Assignees
Labels
bug Indicates an unexpected problem or unintended behavior

Comments

@S-Man42
Copy link

S-Man42 commented Jan 17, 2022

Hi,

I have an input like

8E

After I put this into the parser for evaluation, the parser instantly converts it into

e({8.0})

Is there a way to suppress this behaviour? I expect to get an error (unparseable or something) instead.

@fkleon fkleon added the bug Indicates an unexpected problem or unintended behavior label Jan 17, 2022
@fkleon
Copy link
Owner

fkleon commented Jan 17, 2022

Hi @S-Man42 this sounds like a bug in the parser. The only allowed syntax should either e(8) or e^8.

@S-Man42
Copy link
Author

S-Man42 commented Mar 24, 2022

Hi, any news on this? :)

@Birdy2404
Copy link

Hi,
Are there any news for this topic? Would appreciate your bug fix.
Best regards
Andreas

@S-Man42
Copy link
Author

S-Man42 commented Jan 17, 2025

Still having this issue :(

@fkleon
Copy link
Owner

fkleon commented Jan 30, 2025

@S-Man42 I've had a brief look a this and can't reproduce with the example you gave originally. This test seems to be passing: b593272

Can you check if you've missed something?

@S-Man42
Copy link
Author

S-Man42 commented Jan 30, 2025

Interesting. I still have this issue. I will have a deep dive into my code and let you now asap

@S-Man42
Copy link
Author

S-Man42 commented Jan 30, 2025

Yes, I can totally reproduce it with math_expressions 2.6.0 Here is a screenshot from my app. As you can see "8A" is not giving anything, whereas "8E" gives a result. However, I lowerCase the input before I put it into your library. Does this make a difference?

Image

I checked the point in the code where I call your library (https://github.com/GCWizard/GCWizard/blob/c3bb2ed1060584698cd5557646951ac3b4420644/lib/tools/formula_solver/logic/formula_parser.dart#L472):

Image

The print statements give

I/flutter ( 3545): 8e
I/flutter ( 3545): 2980.9579870417283

Stepping into your code definitely shows that the parser finds a TokenType.EFUNC:

Image

@fkleon
Copy link
Owner

fkleon commented Jan 31, 2025

Thanks for the additional detail. Based on your sample code I was able to reproduce this as well with the lowercase version of the expression (8e).

@fkleon fkleon self-assigned this Feb 1, 2025
@fkleon fkleon mentioned this issue Feb 1, 2025
@fkleon
Copy link
Owner

fkleon commented Feb 1, 2025

Hi @S-Man42, I've uploaded a pre-release version 2.7.0-rc.1 to pub which includes the new parser implementation from #89. Could you give that a try and report back?

You'll have to change the instantiation of the parser as follows to make use of the new implementation:

// math-expressions v2.6.0
Parser parser = Parser();
// math-expressions v2.7.0-rc.1
ExpressionParser parser = GrammarParser();

It should be fully backwards compatible except for some minor features that I haven't finished yet (implicit multiplication, and parsing of n-th root function).

@S-Man42
Copy link
Author

S-Man42 commented Feb 1, 2025

I'll check asap.

However, we cannot use it atm if nth root is not working. Since our tool documented the nrt formula we need to wait for its implementation to provide full backwards compatibility. Can you estimate its implementation?

@fkleon
Copy link
Owner

fkleon commented Feb 2, 2025

@S-Man42 I've now published 2.7.0-rc.2 which includes support for parsing the nth root function in the new parser. It also fixes the minimum dependency constraint for petitparser which could have led to a build error when the installed dependency version was too old.

Another caveat I realised is that variable/function naming is more restrictive in the new parser implementation. The allowed pattern for identifiers is: Starting with a letter or $, followed by any number of word characters (letter or digit).

If that's an issue for you I'll consider some alternative options.

@S-Man42
Copy link
Author

S-Man42 commented Feb 2, 2025

Hi, thanks for your. However I don't get it running because I have a dependency version conflict between your embedded petitparser and our used flutter_localizations:

Because every version of flutter_localizations from sdk depends on meta 1.15.0 and petitparser >=6.1.0 depends on meta ^1.16.0, flutter_localizations from sdk is incompatible with petitparser >=6.1.0.
And because math_expressions >=2.7.0-rc.2 depends on petitparser ^6.1.0, flutter_localizations from sdk is incompatible with math_expressions >=2.7.0-rc.2.
So, because gc_wizard depends on both flutter_localizations from sdk and math_expressions ^2.7.0-rc.2, version solving failed.

However, for testing purposes and giving you some feedback, I did a dependency override.

Ok, well. I recognized some failing tests now in my engine:

  1. Currently I was defining some constants like π and φ. These are not interpreted anymore (the comments in the code are quite old but maybe interesting for you as well ;-) ). Not supporting these would be a show-stopper for us as our users use them very much:
static const Map<String, double> CONSTANTS = {
    'ln10': ln10,
    'ln2': ln2,
    // 'log2e': log2e,    // not supported due to a problem by the math expression lib: https://github.com/fkleon/math-expressions/issues/35
    // 'log10e': log10e,  // not supported due to a problem by the math expression lib: https://github.com/fkleon/math-expressions/issues/35
    'pi': pi,
    '\u03A0': pi,
    '\u03C0': pi,
    '\u220F': pi,
    '\u1D28': pi,
    'phi': _PHI,
    '\u03A6': _PHI,
    '\u03C6': _PHI,
    '\u03d5': _PHI,
    '\u0278': _PHI,
    // 'sqrt1_2': sqrt1_2, // not supported due to a problem by the math expression lib: https://github.com/fkleon/math-expressions/issues/35
    'sqrt2': sqrt2,
    'sqrt3': _SQRT3,
    'sqrt5': _SQRT5,
  };
    for (var constant in CONSTANTS.entries) {
      _context.bindVariableName(constant.key, Number(constant.value));
    }
  1. The input values for functions are not as permissive as before. This worked so far:
sin(0.5)
sin(.5)
sin(5)
sin(5.0)
sin(5.)

The second and the fifth case give an error now.

@fkleon
Copy link
Owner

fkleon commented Feb 2, 2025

@S-Man42 Thanks for testing this release!

The dependency conflict is a about a transitive dependency of flutter_localizations and petitparser (the meta package), so I'm afraid not much I can do about it. Hopefully flutter_localizations will catch up soon and support the newer version of meta. This is one of the reasons I am always hesitant to add dependencies if I can do without.

  1. I agree that it would be nice to support unicode for constants, so that you can use symbols like π. However full unicode support is only coming in petitparser v7.0.0. Would it be sufficient for you to have limited support, e.g. only a list of symbols as per your CONSTANTS?

  2. That should be possible. I'll see what I can do.

Sorry, no ETA on these as I only am able to work on this library in my spare time.

@S-Man42
Copy link
Author

S-Man42 commented Feb 2, 2025

I am not quite sure what you mean with your first question. I basically need Pi and Phi in the crazy four (?) versions I listed in the CONSTANTS definition I mentioned :) . Currently I don't see more special characters here.

@fkleon
Copy link
Owner

fkleon commented Feb 3, 2025

@S-Man42 I've implemented some additional default constants, and also the ability to specify custom constant symbols that the parser will recognise (which can be any string and don't have to match the identifier naming schema).

In v2.7.0-rc.3you can now do something like:

ExpressionParser parser = GrammarParser(ParserOptions(constants: {
      'Π': math.pi,
      'Φ': 1.6180339887,
    }));

See the CHANGELOG

@fkleon fkleon closed this as completed in #89 Feb 5, 2025
@S-Man42
Copy link
Author

S-Man42 commented Feb 6, 2025

Works perfectly now regarding the constants! Thanks. Only the missing leading or trailing zero-issue is still not working. It would be a very nice feature, but is not a breaker on our side.

@S-Man42
Copy link
Author

S-Man42 commented Feb 6, 2025

Btw: Is it possible to remove fixed variables? I think e is an edge case I would not like to have in my parser. e() as function is already working and is used that way. However e has another meaning in our case. So can I switch it off?

Edit: Explanation:
We deal with textual variables like A + B. And it is very common, that one uses E as well, the first 10 letters or so are very common. We actively return the input formula if we find an unset variable and your parser could not deal with the formula. So currently we returned the given A + E as 1 + E if, e.g., A is set to 1 and E remains unset. Now, since e is now a valid constant in your parser, it returns 3.718... instead which could be quite confusing and is in 99% unexpected. So, I'd like to manually switch off the usage of e if possible. Is this doable in any way?

Edit: I found a way! :)

class MyGrammarParser extends GrammarParser {
  final ParserOptions options;

  MyGrammarParser(this.options) : super(options) {
    constants.remove('e');
  }
}

@fkleon
Copy link
Owner

fkleon commented Feb 7, 2025

@S-Man42 Thanks for testing! I hadn't considered that users may want to opt out of the built-in constants.

The workaround you posted looks good to me, I will add it to the official documentation 😄

Edit: Added in 2c21520 - slightly more concise way:

class MyGrammarParser extends GrammarParser {
  MyGrammarParser([super.options]) {
    constants.remove('e');
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants