Mathjs: Move Unit.js into a standalone npm library

[josdejong]

Just floating an idea here: the Unit.js class is a powerful piece of functionality on it's own, separate from math.js. It could be interesting to think about splitting it from math.js into it's own npm library, similar to the libraries Decimal.js, Fraction.js and Complex.js used by mathjs under the hood. Of course Unit.js should keep it's injection of methods like add, multiply, Complex, etc to keep it flexible and integrable).

What would we gain with it?

• Modularity. People may not need mathjs but just need to do some calculations with units.
• Focus. Moving it into a separate library can help focusing. Unit.js has it's own share of complicated problems, and being a part of a larger library (mathjs) doesn't make it easier I think.

@ericman314 do you have some thoughts in this respect?

[ericman314]

Sounds like a great idea. Not sure how difficult it would be. I'll give it some thought.

[harrysarson]

We could nab the @mathjs namespace on npm for it?

[josdejong]

@harrysarson I guess we could, though I think it will be more powerful if Unit.js would become a really standalone library, and not sort of a child project still coupled to mathjs.

[ericman314]

Sounds like a real challenge, but it feels like the right direction to take it. Where do we go from here? Do we create a new repo?

[josdejong]

I think it involves the following steps:

1. Create a proof of concept, get a clear view on the challenges
2. Find someone who likes to pick up this challenge and becomes the "owner" of the project. Since large part of the code was developed by @ericman314 it would make sense to me if it becomes his library, but it totally depends on if you, Eric, like it and feel like you have enough time to create and maintain the library.
3. Decide upon a name for the library, create an empty git repo and claim the name on npm.
4. Think about an API for the library:
   
   
   
   • Creation of units: class new Unit(...) or factory function unit(...)?
   
   
   • Dependency injection for the functions like add, multiply, ...
   
   
   • Configuration
   
   
   • Functions as methods like new x.add(b) or standalone functions like add(a, b) ? Functions as methods, like Decimal.js, Complex.js, and Fraction.js do, have as advantage a nice chaining API. The disadvantage though is that you can't do tree shaking, you have to bundle the complete class with all methods even if you just use a few.
   
   
5. Migrate and implement the new library. Functions, docs, examples, unit tests, ...
6. Start using the new library in mathjs

[harrysarson]

I like the idea of making mathjs into a monorepl with one github repo containing multiple npm packages. Although this approach has downsides too.

[josdejong]

Yes, it's also possible to spin multiple npm packages from the current mono repo.

The reason I'm interested in separating Unit.js is that, like say Decimal.js, it becomes even more powerful if it's focused and standalone. Having a mono-repo typically results in the units of functionality becoming entangled. It is harder to make the "right" decisions for Unit.js when the library when it is a small gear in a big clockwork. Putting it as a separate library can open up a whole new world of opportunities, just like mathjs did when I did split it from the Math Notepad UI.

I see the future of mathjs as being an integrated environment for mathematics in JavaScript: a "hub" merging a lot of different data types together (matrices, units, complex number, ...), and an expression parser making it convenient to work with mathematics in JavaScript. So, moving Unit.js (and also the Matrix classes) out of mathjs would help mathjs getting more focused, and will probably lead to an API that also makes it easier to extend mathjs with more data types (like typed arrays).

[harrysarson]

That makes a lot of sense 👍

[ericman314]

Sure, I'll take ownership. Sounds like a fun adventure. That part I'm unsure how to do would be injecting other types like Complex and Decimal into the new units module.

[josdejong]

Cool Eric :sunglasses:

There is no need to know all answers beforehand already, one step at a time.

Have a look at the Unit.js file in the modular_architecture branch, I think the new injection mechanism is much cleaner, can give some inspiration on how we could solve it for the new Unit.js library.

[ericman314]

All finished: https://github.com/ericman314/unitmath

Just kidding, all it does is output Hello, World!

[ericman314]

I just checked out the dependency mechanism in the modular_architecture branch. It looks great! It kind of reminds me of Angular's dependency injection. However, with unitmath I would like to try and follow the mantra of "do one thing and do it well." Math.js's strength is in managing dependencies and bringing all the different libraries together. I really would not want to have to repeat that mechanism with unitmath, as 99% of use cases in which unitmath will be used alone would not require dependencies on Fraction, Decimal, Complex, etc. I wouldn't want to bundle all of that extra code when it would rarely be used.

Instead, perhaps it would be enough to just allow users (and math.js) to override unitmath's internal add, multiply, round, etc., in order to support different types. unitmath would handle everything with the units, but the user would supply his own types and arithmetic methods for those types. A user's overriding methods could then perform operations between different types using type checking. Do you think that amount of flexibility would be enough for math.js to work with?

[ericman314]

I've written a proposed API for the new library. If you like we can continue the discussion there: https://github.com/ericman314/unitmath/issues/1

[ericman314]

@josdejong, I've written a proposed API for extending the new unitmath library with custom types. Do you think that this API would work with mathjs? (https://github.com/ericman314/unitmath#extending-unitmath)

[josdejong]

Man I love it already @ericman314! The API looks really clean and simple.

I'm glad you go for immutable and factory functions instead of constructors with new :). I think your approach with custom functions add, mul by allowing to _optionally_ override them but have a working solution by default. Setting config looks really elegant!

I'm not sure about the name "extendType". maybe call it "arithmetic" or something? Or simply flatten the config, that may be cleaner and easier to remember, like u = unit.config({ add: ..., mul: ... }).

About number/BigNumber/Fraction, etc: I think that the library does not need knowledge about these specific numeric types, but simply allow passing your own parseNumber function via the config, which enables for example mathjs to parse a string into a BigNumber or Fraction depending on the config of mathjs. I'm not sure how to go about Complex though since right now we have a complex unit VAR in mathjs.

I was thinking about having all functions as methods, which doesn't allow for tree-shaking: that may not be an issue in practice since the number of methods is relatively limited and the implementation is small compared to the rest of the library. So please go ahead with chained methods as described in your proposal :)

Are you still open for finding a nice fancy funny catchy name for the library? If so I will give it some thought.

[ericman314]

Thanks for the feedback! You are right that flattening the config would be simpler. No need to overcomplicate things. I actually do find it difficult to use nested config options (think chart.js). The cloning implementation would be a lot simpler, too. Maybe prefix each with "custom", to show that it's a custom operator? So, customAdd, customMul, and so on. I'll think on that.

I also thought about a parseNumber config option, (or maybe we'd call it customParse?) to use with custom types, but then I thought that it might be just as easy for the custom type library to do the parsing, then pass the custom type and unit string into Unitmath. Of course it would be no problem at all to support the parseNumber config option, but since the API support parsing a single string that contains both the value and the units, it might get tricky because the custom parser would need to consume only the numeric portion and leave the units untouched. So we would have that stipulation, otherwise you would have to use the two-argument unit(value, unitStr) when using a custom type.

Yes it's too bad about the complex unit VAR. I'm afraid it will be the first casualty resulting from splitting Unit.js into its own library. I can't think of a good way to include it. It was a bit of a "toy" unit that I honestly don't think gets that much use. And personally I think it even leads to confusion, because a complex quantity is formatted using a real valued number and a complex unit. I would want to be sure 100% whether my quantity was a complex number or not; I would want to see the i.

I'm definitely open to catchy names. I tried variants of Unit.js but they were too similar to existing libraries. I settled on Unitmath because it hints at what the library does--math with units. It's also not too hard to remember, and it's quick to type. Unfortunately, it is painfully boring. To me, it's OK for a really popular library to have a weird, unrelated name (like electron), but I tend to put more trust in smaller, less known libraries if the name actually says what it does.

[ericman314]

I just altered the capitalization in the docs to UnitMath and I like it much better now. (The package name will remain lowercase.)

[josdejong]

Sounds good going with flat config options. Maybe the "custom" prefixes are a bit superfluous too: any config option you set is per definition custom since it deviates from the default :).

I think you're right about parseNumber: it may be redundant since you can pass any numeric type when constructing a unit. KISS?

It makes sense to simply leave the VAR unit out of the library, and only create it as a custom unit in mathjs.

I totally agree that a boring but clear name is better than something vague. I will give the naming some thought. It's unfortunate that "unit" conflicts with unit testing libraries.

[ericman314]

Version 0.2.1 of UnitMath was just released. It is nearing a stable API. I haven't tested it with custom types yet, but trying to integrate it into Math.js might be the perfect opportunity to put it through its paces. What branch should we be basing this work on?

[ajbouh]

Hope this isn't too far off topic for this, but I have a sort of meta custom type that I've put together for dealing with sums of different unit types. I'd love to contribute it back upstream if there's interest.

[ericman314]

Absolutely, you can find the project here: https://github.com/ericman314/UnitMath

[josdejong]

That's awesome news @ericman314 :sunglasses: ! You can start a new feature branch based on develop, like feature/unitmath.

[ajbouh]

Thanks, I just added https://github.com/ericman314/UnitMath/issues/13

[josdejong]

@ericman314 v6 is about ready to release, I hope to publish the last beta version today. After that I want to write a blog post about it, work on some nice-to-haves. The real release can be done in one or two weeks from my point of view, but it would be great to have unitmath integrated too.

What is the status of unitmath and when do you realistically think we could have it integrated in mathjs? Is it worth to wait for it? If so, with all pleasure! mathjs@6 is under way for more than a half year now, a few weeks extra is no problem at all. I don't want to put pressure on unitmath though, just get our planning aligned.

[ericman314]

UnitMath is getting very close. Since mathjs will be its biggest user at first, having it fully integrated and working would be a good indicator that it has reached a stable api. Otherwise I'm afraid it would just increment minor versions forever without reaching v1. I'll need some help integrating it though, as I'm unfamiliar with mathjs@6's new architecture. A couple weeks might be a little fast, but we'll try.

[harrysarson]

I have some time on my hands from Friday onwards and happy to help piping UnitMath into mathjs

[ajbouh]

Will the new architecture make it possible to define units limited to a
specific scope?

On Sun, May 26, 2019, 08:47 Harry Sarson [e-mail] wrote:

I have some time on my hands from Friday onwards and happy to help piping
UnitMath into mathjs

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/josdejong/mathjs/issues/1486?email_source=notifications&email_token=AAABZHJUNRTTOCNDTBENKTDPXKWKFA5CNFSM4HH52ZY2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWIIKJY#issuecomment-496010535,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAABZHNSPRL5BLGXIZIM5ITPXKWKFANCNFSM4HH52ZYQ
.

[ericman314]

Yes, that should be possible.

[josdejong]

Cool sounds good Eric! Coming Tuesday or Wednesday I can setup a branch starting to import unitmath and replace the old Unit class. From there the three of us (Eric, Harry, me) can work on the branch fixing broken tests and whatever comes along. Does that sound like a plan?

[josdejong]

Here we go: https://github.com/josdejong/mathjs/tree/unitmath (almost 200 tests failing at this moment, probably just a few causes)

[ericman314]

Awesome! I'll get to work on it.

[ericman314]

Oh and it's probably more than just a few causes, I made quite a few changes to the library. Hopefully they are all improvements!

[ericman314]

Here are a few things items I've found:

• [ ] All of the createUnit tests are failing. UnitMath's custom unit definitions were totally redone. We'll need to create a wrapper for createUnit that reconfigures UnitMath with the new unit definition(s).
• [x] A unit's value is now stored in the "denormalized" format rather than the "normalized" (SI) format, and those functions are private now. Will have to modify multiplyScalar.js and divideScalar.js.
• [ ] Some tests are failing because of incorrect formatting. We'll need to pass format options into UnitMath's formatter. For custom types, UnitMath calls toString on the unit's value while formatting, so maybe we can use that. Actually, there is a bug in UnitMath that means toString is not called if the custom type is a number, so maybe that fix would partially solve this.
• [ ] It looks like units are being created with numbers, not custom types (BigNumber, Fraction). Not sure where to look for fixing that.
• [ ] The old Unit.js had a toNumeric(unitString). In the new API, you would use to(unitString).value. Is the toNumeric method used in other parts of mathjs?
• [ ] UnitMath doesn't output parentheses by default, but we might want to set that option to true to match the old behavior.

[ericman314]

It looks like units are being created with numbers, not custom types (BigNumber, Fraction).

I think this is because the conv function that converts to the custom type is configured according to math.config, so no matter what type of value you give to the unit constructor, it converts it to the type given by math.config. For example, if I config UnitMath for using Fractions, and then I call unit(BigNumber), the unit constructor will call conv which will convert my BigNumber to a fraction. Probably not what we want. In the constructor, conv could just keep the same type that it is given (apart from strings, which it should convert according to the math.config) but there are other places in UnitMath where numbers have to be converted, and there's no way to tell conv what type they should be converted to.

Unless, here is my crazy idea: we could have UnitMath call conv with a second argument, which is the prototype of some reference value (such as this.value) that it should be converted to. It's like saying, "Will you please convert this number to your custom unit, and for best results, make it a Decimal." The second argument can be ignored if you are only using one type.

The other possibility is keeping a separate UnitMath instance for each type, and then choosing which one to invoke based on the type of the value that will be constructed. I imagine that could get unwieldy, but the advantage would be that you'll have a complete set of compiled units in each type, not just the type configured in math.config. But it would be even harder to keep all the options straight, and I'm not sure what would happen if you tried to operations between units of different types, since they would be from different UnitMath instances.

[josdejong]

:+1: it will be a puzzle indeed.

The conv function that I implemented in the feature branch should indeed leave inputs as they are except when the input is a string, I hadn't thought about that. I'm not sure about your crazy idea though. Except for facilitating a custom parse function, I think unitmath should stay away from handling custom types - it should just accept what comes out of the conv function and that's it. Does that make sense?

As for methods like toNumeric, which don't exist anymore with the new library: It is fine to remove them, but for a smooth transition, maybe we can attach them to the unitmath instance that we create, with a deprecation warning (warnOnce). This deprecation code should go in mathjs, not in unitmath.

[ericman314]

Yes, I think unitmath should remain blind to how the custom types work. I just am not sure how to make it work. Let's say unitmath has a function called timesTwo:

class Unit {
  timesTwo () {
    return this.mul(options.type.conv(2))
  }
}

Mathjs can't know, inside conv, what type to convert the number 2 to. Maybe the answer is to leave it as a number and let math.multiplyScalar take care of it. Anyway, the crazy idea is to include an instance of a custom type, the type prototype, or this itself, for the rare instance in which the conv function has to convert conditionally based on the type of unit:

class Unit {
  timesTwo () {
    return this.mul(options.type.conv(2, this))
    // OR
    return this.mul(options.type.conv(2, this.value))
    // OR
    return this.mul(options.type.conv(2, this.value.prototype))
  }
}

I don't like this solution. It is true that math.js's conv function could then convert to the correct type, but it would only work in contexts with this. It wouldn't work when compiling the unit definitions. It is a bit of a workaround though, and probably could cause bugs since it's a little bizarre and tailored just for math.js.

I do believe there is a more sound solution, maybe if we try a few things we'll come up with something better.

Good idea for toNumeric, by the way.

[josdejong]

And how about something like this?

class Unit {
  timesTwo () {
    return this.mul(options.type.conv('2')) 
  }
}

[ericman314]

I was thinking about this more:

Leave it as a number and let math.multiplyScalar take care of it

If math.js tries to multiply a Fraction and a number, the result would be a Fraction, wouldn't it?

[ericman314]

I've had the best luck with this for conv:

conv: (value) => typeof value === 'string' ? numeric(value, config.number) : value,

I think at this point most of the tests are failing due to formatting issues. There are still some problems with implicit conversion of numbers to BigNumbers. Perhaps this could be fixed by using your suggestion, Jos, of recasting all the hard-coded numbers in UnitMath to strings. Built-in units would store their numbers as strings, which conv would convert to the appropriate type on a case-by-case basis. This would incur a lot of overhead for most users of the library, which would only use the default floating-point numbers. If you think this has a good chance of working, I'll open an issue over at UnitMath and talk about the best way to do it.

[josdejong]

If you want to go puristic and fully support custom numeric types, I think unitmath should parse any numeric value from string into a custom numeric type. This could yield a performance penalty, though I _think_ that's negligible, especially when applying say memoization or some other caching technique.

However, practically seen, most units do not have a precision higher than what can be expressed with a regular number, so we don't lose precision when using numbers internally to store the values of units. The current conv function already offers the flexibility to turn a number into a BigNumber if you would like to do that.

I like your latest solution for conv in mathjs, I think that just does the trick. I think it's best to stay pragmatic here. And indeed Fraction * number results in a fraction in mathjs already.

Do you think conv is the best name for the function? In my head parse is a better fit, but it's up to you :)

[ericman314]

Yeah I'd like that. I think what I'll try to do is define hard-coded numbers as strings instead. That should hopefully avoid the errors we've been getting converting numbers to Fraction and BigNumbers.

Then it could silently convert those strings to the custom type and cache them for a performance improvement.

And by default, the conversion function will continue to support either numbers or strings, for maximum flexibility in defining new units.

And finally, since the minimum requirement for the conversion function is that it can parse a string, parse is indeed a better name.

[ericman314]

Already running into trouble. Consider this function in denormalize() that converts the value of a unit from base quantities (for example, the value 0.0254 is converted to 1 in 1 inch)

function denormalize(unitPieces, value, type) {
  let result = value
  for (let i = 0; i < unitPieces.length; i++) {
    unitValue = type.conv(unitPieces[i].unit.value)
    unitPrefixValue = type.conv(unitPieces[i].unit.prefixes[unitPieces[i].prefix])
    unitPower = type.conv(unitPieces[i].power.toString())
    result = type.div(result, type.pow(type.mul(unitValue, unitPrefixValue), unitPower))
  }
  ...

Now consider the following test:

const unit3 = new Unit(math.bignumber(14.7), 'lbf')
const unit4 = new Unit(math.bignumber(1), 'in in')
const unitD = unit3.div(unit4)
assert(math.isBigNumber(unitD.value))

The error occurs at unit3.div(unit4), when denormalizing 1 in^2. The variables unitValue, unitPrefixValue, and unitPower are all numbers, on account of their being converted from strings to the type configured by math.config.number, which means type.pow(type.mul(unitValue, unitPrefixValue), unitPower) is also a number (1550.0031000062002). Then, type.div(result, ...) throws an error because the number has too many digits, so it can't be implicitly converted to a BigNumber.

So after seeing the intricacies involved, I'm a little more hesitant about converting everything to strings. Do you have any other ideas?

[ericman314]

What if we created wrappers for div, mul, etc. in function/unit.js. that inspect the types of the arguments and promote them as necessary to avoid the implicit conversion error? Although that is what typed-function does already, except it doesn't allow the implicit conversion from numbers with > 15 digits.

[josdejong]

Creating wrappers in function/unit.js where needed is definitely a good idea wherever we need to solve conflicts between mathjs and mathunit.

If working from strings internally gives too much complications, it may be best to just work from numbers internally instead. Or does that not solve these complications? The old Unit.js class in mathjs used numbers internally, which worked fine right?

[ericman314]

Just gave it a try, and fewer tests are failing than ever before! We may be on to something.

  const promoteArgs = (fn, ...args) => {
    let types = {}
    args.forEach(a => { types[a.type || typeof a] = true })
    const numTypes = Object.keys(types).length
    // We expect to have these types:
    // number
    // Complex
    // BigNumber
    // Fraction

    if (numTypes === 1) {
      // No alteration is necessary
      return fn(...args)
    } else if (numTypes === 2) {
      // May need to convert one or more of the arguments
      if (types.hasOwnProperty('number')) {
        if (types.hasOwnProperty('Complex')) {
          // Convert all args to Complex
          return fn(...args.map(a => typeof a === 'number' ? new Complex(a, 0) : a))
        } else if (types.hasOwnProperty('Fraction')) {
          // Convert all args to Fraction
          return fn(...args.map(a => typeof a === 'number' ? new Fraction(a) : a))
        } else if (types.hasOwnProperty('BigNumber')) {
          // Convert all args to BigNumber
          return fn(...args.map(a => typeof a === 'number' ? new BigNumber(a) : a))
        }
      }
    }

    // All valid paths should have returned by now
    // Throw this error when debugging, or for more consistent error messages, try it anyway and let typed.js throw
    throw new Error('unit.js attempted to perform an operation between the following incompatible types: ' + Object.keys(types).join(', '))
    //return fn(...args)
  }

[ericman314]

I think for the most part the type problems have been resolved. Do you want pull the changes so far into the unitmath branch? https://github.com/ericman314/mathjs/tree/unitmath-test-fixes

[josdejong]

That's good news! Yes feel free to merge your changes into the unitmath branch and continue fixing and tweaking there.

[ericman314]

There was an issue with round-off error due to floating point arithmetic that was then getting proprogated into Fractions, so I caved and added a second parameter to conv to provide a hint as to which type to convert to. Now that test passes because the arithmetic is done using Fractions, not floating point. I don't particularly like this solution, and I'm leaving that second parameter undocumented and subject to removal if we ever figure out a better way to do it. Math.js is a unique use-case of UnitMath in that there are several different custom types, and that second parameter provides the information needed to convert a number or string into the correct type depending on the context.

I've pushed all my commits into the unitmath branch. UnitMath is now @0.8.2. Here are the remaining issues I have identified which I could use a little more clarification or help on:

• No comparison method for complex numbers, so choosePrefix doesn't work, and no way to conditionally skip that function without creating a separate unitmath config for complex numbers**
• toNumeric, toNumber, I can't remember if we need these.
• The fraction constructor is giving some InvalidParameter errors
• Once a unitmath is configured, its unit definitions cannot be modified, so what's the best way to wrap a configuration change inside of the createUnit function?
• Can't figure why unit.exists is not a function, maybe I'm missing something obvious.

** About the complex comparison method--if we create a separate unitmath instance for each type, not only could we set prefix: 'never' for that instance, we could also eliminate the need for the second parameter in conv. Then math.js's unit constructor would wrap the four types and return the correct unitmath instance depending on the type. Might be messy though.

[ericman314]

I'm trying the multiple instances of unitmath approach, and I'd like to be able to call Unit from within divideScalar.js (and others) so that the wrapper around the Unit constructor can select the correct instance of unitmath in case the value type needs to be upgraded. But I'm having trouble importing Unit into divideScalar.js. When I add Unit to the dependencies, I get some kind of stack overflow. Does this mean that circular dependencies are not allowed? Source: https://github.com/ericman314/mathjs/tree/unitmath-single-type-instances

  ...
    at Object.valueResolver [as divideScalar] (/home/eric/Documents/mathjs/src/utils/object.js:217:18)
    at /home/eric/Documents/mathjs/src/core/function/import.js:327:44
    at Array.forEach (<anonymous>)
    at forEach (/home/eric/Documents/mathjs/src/core/function/import.js:314:10)
    at Object.valueResolver [as divide] (/home/eric/Documents/mathjs/src/utils/object.js:217:18)
    at /home/eric/Documents/mathjs/src/core/function/import.js:327:44
    at Array.forEach (<anonymous>)
    at forEach (/home/eric/Documents/mathjs/src/core/function/import.js:314:10)
    at Object.valueResolver [as unit] (/home/eric/Documents/mathjs/src/utils/object.js:217:18)
    at /home/eric/Documents/mathjs/src/core/function/import.js:327:44
    at Array.forEach (<anonymous>)
    at forEach (/home/eric/Documents/mathjs/src/core/function/import.js:314:10)
    at Object.valueResolver [as divideScalar] (/home/eric/Documents/mathjs/src/utils/object.js:217:18)
    at /home/eric/Documents/mathjs/src/core/function/import.js:327:44
    at Array.forEach (<anonymous>)
    at forEach (/home/eric/Documents/mathjs/src/core/function/import.js:314:10)
    at Object.valueResolver [as divide] (/home/eric/Documents/mathjs/src/utils/object.js:217:18)
    at /home/eric/Documents/mathjs/src/core/function/import.js:327:44
    at Array.forEach (<anonymous>)
    at forEach (/home/eric/Documents/mathjs/src/core/function/import.js:314:10)
    at Object.valueResolver [as unit] (/home/eric/Documents/mathjs/src/utils/object.js:217:18)
    at /home/eric/Documents/mathjs/src/core/function/import.js:327:44
    at Array.forEach (<anonymous>)
    at forEach (/home/eric/Documents/mathjs/src/core/function/import.js:314:10)
    at Object.valueResolver [as SymbolNode] (/home/eric/Documents/mathjs/src/utils/object.js:217:18)
    at /home/eric/Documents/mathjs/src/core/function/import.js:327:44
    at Array.forEach (<anonymous>)
    at forEach (/home/eric/Documents/mathjs/src/core/function/import.js:314:10)
    at Object.valueResolver [as FunctionNode] (/home/eric/Documents/mathjs/src/utils/object.js:217:18)
    at /home/eric/Documents/mathjs/src/core/function/import.js:327:44
    at Array.forEach (<anonymous>)
    at forEach (/home/eric/Documents/mathjs/src/core/function/import.js:314:10)
    at Object.valueResolver [as parse] (/home/eric/Documents/mathjs/src/utils/object.js:217:18)
    at /home/eric/Documents/mathjs/src/core/function/import.js:327:44
    at Array.forEach (<anonymous>)
    at forEach (/home/eric/Documents/mathjs/src/core/function/import.js:314:10)
    at Object.valueResolver [as Help] (/home/eric/Documents/mathjs/src/utils/object.js:217:18)
    at Object.Help (/home/eric/Documents/mathjs/test/expression/Help.test.js:6:19)
    at Module._compile (internal/modules/cjs/loader.js:702:30)
    at Module._compile (/home/eric/Documents/mathjs/node_modules/pirates/lib/index.js:99:24)
    at Module._extensions..js (internal/modules/cjs/loader.js:713:10)
    at Object.newLoader [as .js] (/home/eric/Documents/mathjs/node_modules/pirates/lib/index.js:104:7)
    at Module.load (internal/modules/cjs/loader.js:612:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:551:12)
    at Function.Module._load (internal/modules/cjs/loader.js:543:3)
    at Module.require (internal/modules/cjs/loader.js:650:17)
    at require (internal/modules/cjs/helpers.js:20:18)
    at /home/eric/Documents/mathjs/node_modules/mocha/lib/mocha.js:330:36
    at Array.forEach (<anonymous>)
    at Mocha.loadFiles (/home/eric/Documents/mathjs/node_modules/mocha/lib/mocha.js:327:14)
    at Mocha.run (/home/eric/Documents/mathjs/node_modules/mocha/lib/mocha.js:804:10)
    at Object.exports.singleRun (/home/eric/Documents/mathjs/node_modules/mocha/lib/cli/run-helpers.js:207:16)
    at exports.runMocha (/home/eric/Documents/mathjs/node_modules/mocha/lib/cli/run-helpers.js:300:13)
    at Object.exports.handler.argv [as handler] (/home/eric/Documents/mathjs/node_modules/mocha/lib/cli/run.js:296:3)
    at Object.runCommand (/home/eric/Documents/mathjs/node_modules/mocha/node_modules/yargs/lib/command.js:242:26)
    at Object.parseArgs [as _parseArgs] (/home/eric/Documents/mathjs/node_modules/mocha/node_modules/yargs/yargs.js:1087:28)
    at Object.parse (/home/eric/Documents/mathjs/node_modules/mocha/node_modules/yargs/yargs.js:566:25)
    at Object.exports.main (/home/eric/Documents/mathjs/node_modules/mocha/lib/cli/cli.js:63:6)
    at Object.<anonymous> (/home/eric/Documents/mathjs/node_modules/mocha/bin/_mocha:10:23)
    at Module._compile (internal/modules/cjs/loader.js:702:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:713:10)
    at Module.load (internal/modules/cjs/loader.js:612:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:551:12)
    at Function.Module._load (internal/modules/cjs/loader.js:543:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:744:10)
    at startup (internal/bootstrap/node.js:238:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:572:3)

[josdejong]

We may indeed need to adjust the unit tests of mathjs to the new way that unitmath works.

Does this mean that circular dependencies are not allowed?

That's correct. I was able to remove most circular dependencies in mathjs. In some cases I moved functionality into separate util files. In some cases I was able to keep a dependency on say BigNumber or Unit implicit by accessing it's constructor from a passed Unit instance itself, see for example cos.js.

I will have a more thorough look into the open issues you mention coming days, see where I can help.

[ericman314]

Yes I saw that trick you used in unit.js of accessing the constructor from an instance. Unfortunately, in my unitmath-single-type-instances branch, there are 4 constructors and the Unit function itself chooses the correct one to use. I know there is a right way to do this, we just need to find it.

[josdejong]

1. About conv: it indeed feels dirty to provide this hint as to which type to convert to, and the promoteArgs function also feels relatively complicated. I think it will help to first resolve the other open ends, and when everything more or less works, revisit conv and promoteArgs to see if we can find a better solution.
2. About comparison of complex numbers: I can think of a few directions
   
   
   
   • Do we really need to compare (complex) numbers, or would it be enought to uniquely sort them? In case of the latter, we could use sortNatural from mathjs and pass that via the custom type functions.
   
   
   • We have to implement wrapper around the custom type functions comparing numeric values, so that they do "something" meaningful in case of complex numbers instead of throwing an error.
   
   
3. About toNumeric, toNumber: I think toNumeric is the better version of toNumber, and we only need one of the two. Basically, we want to have a way to get the numeric value out of the unit. Does that make sense?
4. Fraction InvalidParameter: let's do some debugging.
5. About configuration changes in createUnit: I think the most logical here is to the factory function optionally dependent on on, which is a listener that can be used for config changes. Just like createUnitFunction does already. Search for occurrences of '?on' in the code base.

6. unit.exists does work, but the failing unit tests use Unit.exists, and Unit is a wrapper around unit giving a deprecation warning that the class Unit doesn't exist anymore. So this should be fixed when changing the unit tests to use math.unit instead.

   I'm not sure if my "trick" to expose the static method exists on every unit: this probably breaks when you do any operation on the unit, which returns you a new unit:

   // expose static exists function
   unit.exists = (singleUnitString) => unitmath.exists(singleUnitString)
   

   This is a trick to get the static method exists exposed on a unit. I guess that breaks when creating a new unit from the existing unit or so? One solution would be to have unitmath expose the function also as a method on every unit. Or else, in SymbolNode where we need it, we need access to our unitmath instance.

7. About creating a new unit from an existing unit: If this cannot be done via the constructor, we could expose the factory function to create a new unit on every unit, we could call it create(...) for example, like:

   const unit = UnitMath.config({ ... })
   const a = unit('5cm')
   const b = a.create('2 inch') // create a unit without needing the unit factory function
   

[ericman314]

Another equally horrific solution is to make the unit wrapper a state
machine that keeps track of the most recent unit type and then coerce
future arithmetic operations to that type. Like I said, horrific. But if we
rule out all the bad ideas, the only left will be the right answer!

Your other ideas sound good. I think I will have time during the next few
days to work on it.

On Mon, Jun 3, 2019, 1:29 PM Jos de Jong [e-mail] wrote:

>

1.

About conv: it indeed feels dirty to provide this hint as to which
type to convert to, and the promoteArgs function also feels relatively
complicated. I think it will help to first resolve the other open ends, and
when everything more or less works, revisit conv and promoteArgs to
see if we can find a better solution.
2.

About comparison of complex numbers: I can think of a few directions

• Do we really need to compare (complex) numbers, or would it be
  enought to uniquely sort them? In case of the latter, we could use
  sortNatural from mathjs and pass that via the custom type functions.
  
  
  
  • We have to implement wrapper around the custom type functions
    
    comparing numeric values, so that they do "something" meaningful in case of
    
    complex numbers instead of throwing an error.
    
    3.
  
  

About toNumeric, toNumber: I think toNumeric is the better version of
toNumber, and we only need one of the two. Basically, we want to have
a way to get the numeric value out of the unit. Does that make sense?
4.

Fraction InvalidParameter: let's do some debugging.
5.

About configuration changes in createUnit: I think the most logical
here is to the factory function optionally dependent on on, which is a
listener that can be used for config changes. Just like
createUnitFunction does already. Search for occurrences of '?on' in
the code base.
6.

unit.exists does work, but the failing unit tests use Unit.exists, and
Unit is a wrapper around unit giving a deprecation warning that the
class Unit doesn't exist anymore. So this should be fixed when
changing the unit tests to use math.unit instead.

I'm not sure if my "trick" to expose the static method exists on every
unit: this probably breaks when you do any operation on the unit, which
returns you a new unit:

// expose static exists functionunit.exists = (singleUnitString) => unitmath.exists(singleUnitString)

This is a trick to get the static method exists exposed on a unit. I
guess that breaks when creating a new unit from the existing unit or so?
One solution would be to have unitmath expose the function also as a
method on every unit. Or else, in SymbolNode where we need it, we need
access to our unitmath instance.
7.

About creating a new unit from an existing unit: If this cannot be
done via the constructor, we could expose the factory function to create a
new unit on every unit, we could call it create(...) for example, like:

const unit = UnitMath.config({ ... })const a = unit('5cm')const b = a.create('2 inch') // create a unit without needing the unit factory function

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/josdejong/mathjs/issues/1486?email_source=notifications&email_token=ABQNHEPETQAKO5UCSKXNN7LPYVWJRA5CNFSM4HH52ZY2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODW2OE5I#issuecomment-498393717,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABQNHEPTZUR4TSQYN5DSYMLPYVWJRANCNFSM4HH52ZYQ
.

[josdejong]

But if we rule out all the bad ideas, the only left will be the right answer!

:joy: that's the spirit.

Yeah it's quite a challenge. It all feels to complicated. Maybe we need some sort of hook allowing us to expose extra methods on units or so. Or maybe we need to accept that the behaviour will not be as it was before and has certain limitations.

[ericman314]

Finally had some time to read through and respond to your comments above:

1. I think I've got the conversions working using the dirty trick with conv. I don't think it's that bad of a hack, but we'll keep an issue open about improving it in the future.

2. For complex numbers, I made yet another wrapper around the custom operators to do abs before comparing them. Considering where the comparison operators are used in UnitMath, this made the most sense.

3. toNumeric: The canonical way to do this in UnitMath is now: u.value or u.to('kg').value or u.to('kg').getValue(). Does the unit type in math.js need to have a function named toNumeric? I'm not sure how to expose a function like that on every instance without hooking into the UnitMath constructor.

4. The Fraction InvalidParameter was because it was trying to parse 1e-10. I added a simple, but probably not robust, fix to this in type/fraction/function/fraction.js that uses parseFloat first, in the event that the input string contains an 'e' character.

5. Sounds good. What happens if the configuration changes and the user continues to use old instances of units created from previous configurations? I guess that problem is fundamental to UnitMath itself as well, that's just the direction we chose to go with the design.

I wonder if a lot of these issues could be addressed by creating a new class in math.js that wraps every method of unitmath. It could expose whatever methods are needed for backwards compatibility. Every method call from anywhere else in the mathjs library gets filtered through this new class, so it can make sure the types are converted correctly. I'm not familiar enough with the new architecture to know the right way to go about doing that, though.

[josdejong]

• 3 toNumeric: sounds good, then this method is simply replaced with u.to('kg').value. Would be neat to attach a toNumeric and toNumber with a deprecation warning.
• 5 I think we can't do anything about that, it's indeed because unitmath is immutable, which brings us a lot of good though this is a limitation.

• 6 We indeed need quite some mathjs specific extension, which currently is tricky to get attached to units. I think a wrapper class around it would make things complicated (both in working with it as well as maintaining it). What may work out nicely is a hook giving the ability to attach extra methods every time a unit is created, like

  const unitmath = UnitMath.config({
    afterCreate: function (unit) {
      // helper properties for type checking in mathjs
      unit.isUnit = true
      unit.type = 'unit'
  
      // expose static methods for easy access inside mathjs
      unit.create = mathunit
      unit.exists = (singleUnitString) => unitmath.exists(singleUnitString)
  
      // deprecated methods
      unit.toNumeric = function (valuelessUnit) {
        return unit.to(valuelessUnit).value
      }
  
      // ...
  
      return unit
    }
  }
  

• 7 Maybe it's good to re-evaluate our decision to go live with mathunit in mathjs@6. The integration is more complicated than I expected beforehand. Maybe it is good to take the time to think these issues trough instead of rushing towards "some" working solution. Give it some time to mature. We could move unitmath integration to mathjs@7. That's not like missing the latest train or so, we can release mathjs@7 any time we want, when ready. What do you think?

[ericman314]

Works for me. I'll add afterCreate and you can go ahead with v6 and we'll get unitmath ready for v7.

[josdejong]

Ok cool :+1: