Parsing unit fails with parentheses and exponents; documentation unclear

[jasonhornsby]

Describe the bug

When trying to parse the unit lbf/(mile/h)^2 a SyntaxError is thrown.

SyntaxError: Unexpected "^" in "lbf/(mile/h)^2" at index 12

This is unexpected since the unit should be valid and math.evaluate() can parse it without issue (see interactive prompt on https://mathjs.org/)

Narrowing down the issue it seems to be related to the ^ after the brackets.

To Reproduce

These tests reproduce the issue.

The original unit

    it('should convert a unit with squared brackets (original)', function () {
      const u = Unit.parse('lbf/(mile/h)^2')
      assert.notEqual(u, null)
    })

Only squared brackets:

    it('should convert a unit with squared brackets (reduced)', function () {
      const u = Unit.parse('(h)^2')
      assert.notEqual(u, null)
    })

[gwhitney]

Thanks for the detailed report! The only thing I am unsure of is the exact specification of the unit "language" that Unit is designed to handle. Your examples work in the mathjs expression language because mathjs is perfectly happy to do arithmetic on "pure" units without numeric values. So perhaps @josdejong or @ericman314 can comment on whether your examples are expected to work directly in the Unit class parser.

[ericman314]

The unit class parser is not as advanced as math.evaluate(). It can't do nested parentheses, multiple / operators, etc. In fact, I think parentheses might be ignored altogether. The parser only looks for a single / to separate the units in the numerator from the units in the denominator. And a ^ has to directly follow a unit--you can't have a ) in between. Hope that helps.

[jasonhornsby]

Thanks, @gwhitney @ericman314 for the fast response.

So if I understand it correctly this is not a bug but rather a limitation of the unit parser. I will close this issue then and change my usage to rely on math.evaluate() instead. Thanks for the help!

[jasonhornsby]

@ericman314 Would it be an idea to adjust the documentation on the https://mathjs.org/docs/datatypes/units.html page? It already includes notes on the limitations of the unit() function so it would be fitting to mention other limitations. Maybe save the next person some of my headache :)

[ericman314]

Hmm. Looking at that page, the documentation on what syntax is supported doesn't agree with what I told you earlier. I might have been wrong. Without digging in deeper, I can't say. It you would like to submit an improvement or clarification for that documentation, I'm sure it would be most welcome.

[gwhitney]

As far as I can see, the documentation is unclear on this point, so I now feel that there is definitely a problem here. Either:

1. The documentation has to be extended to specify that exponents are only allowed directly on individual units, such as m/s^2 but not (m/s)^2; or
2. The unit parser should be enhanced to accept exponents in all appropriate places (in which case I don't think the documentation needs to be changed, because I don't think anyone will be surprised or dismayed if (m/s)^2 is parsed OK by Unit.

Hence, reopening the issue.

[gwhitney]

Of course, to address this, there needs to be a decision between (1) or (2) from the last comment. Again perhaps @josdejong can weigh in or we can wait until Eric has a bit more time to look into this as to which way would be better to go.

[jasonhornsby]

One thing to add to the (1) or (2) discussion:
I have been trying to bypass the limitations with math.unit with math.evaluate and that is causing issues. One example is the unit min. While min^-1 can be parsed by math.unit without issues, math.evaluate fails since min is interpreted as the min function which can't be an argument to pow. As far as I can see there is no straightforward way of doing the conversion of the unit in the description without breaking support for something like this.

[gwhitney]

. One example is the unit min. While min^-1 can be parsed by math.unit without issues, math.evaluate fails since min is interpreted as the min function which can't be an argument to pow

Well, I think this is a separate issue, the clash between certain unit names and standard function names in mathjs expressions. I mean, "1 hr" parses but neither of "1 min" or "1 sec" do (because of the minimum and secant functions). And I really don't know what if anything is to be done about that; you could try to disambiguate by context, but I think that's wading into a quagmire. Right now as far as I know you just have to use other symbols for those units, like "minute" and "s". If you have a specific proposal for when the mathjs expression parser should prefer a unit to a function symbol, feel free to open a separate issue (or if your idea is not quite concrete or fully detailed, maybe open a Discussion about it instead). Thanks!

[josdejong]

Yes good points. I think we should improve the documentation for now, that is very little work.

The parser of Unit is much more limited than the expression parser math.evaluate, and it will probably always be. It is up to Eric to decide what syntax is supported by the parser when we move to UnitMath. A notation like (m/s)^2 sounds quite common, so it may be worth adding it.

[josdejong]

The point about min is indeed tricky, it can be both the function and the unit. You can maybe use the unambiguous minimum instead to stay safe?

[gwhitney]

Well currently in the mathjs expression parser, 'min' and 'sec' are unambiguously minimum and secant, so to get the units you need to write out minute and use either s or second, both of which it recognizes as seconds. But in the Unit parser, min and sec mean minutes and seconds. So there is that inconsistency.

As for whether to extend the Unit parser to handle exponents in more general positions than directly on a unit, I guess @jasonhornsby and I need guidance from someone as to that or to document the limitation. Thanks.

[josdejong]

Yes that is a good point, well explained. Shall we should remove min and sec from Unit to make things at consistent?

1. for the time being I think it would be best to document the limitation in mathjs
2. in the long run, we can maybe extend the parser of UnitMath, but the details of that should be discussed in the library repo of UnitMath.

[gwhitney]

Good, at least it is clear what to do here for this issue at this time: document the quirks in mathjs/Unit interaction as they currently stand. My vote is not to remove min and sec from Unit, just document that math overrides them in expressions, since you can still certainly do math.unit('10sec') and I don't see any reason to disable that.

[josdejong]

👍

since you can still certainly do math.unit('10sec') and I don't see any reason to disable that

Right now Unit.js is still part of mathjs itself, and we can simply remove these units from the code. But as soon as we switch to UnitMath, we can't control that and the issue will be back. So, maybe indeed not a good idea to remove these units and best to document this overriding behavior as clear as we can.