Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Move Unit.js into a standalone npm library #1486

Closed
josdejong opened this issue Apr 23, 2019 · 62 comments
Closed

Move Unit.js into a standalone npm library #1486

josdejong opened this issue Apr 23, 2019 · 62 comments

Comments

@josdejong
Copy link
Owner

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
Copy link
Collaborator

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

@harrysarson
Copy link
Collaborator

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

@josdejong
Copy link
Owner Author

@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
Copy link
Collaborator

ericman314 commented Apr 24, 2019

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
Copy link
Owner Author

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
Copy link
Collaborator

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
Copy link
Owner Author

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
Copy link
Collaborator

That makes a lot of sense 👍

@ericman314
Copy link
Collaborator

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
Copy link
Owner Author

Cool Eric 😎

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
Copy link
Collaborator

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

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

@ericman314
Copy link
Collaborator

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
Copy link
Collaborator

I've written a proposed API for the new library. If you like we can continue the discussion there: ericman314/UnitMath#1

@ericman314
Copy link
Collaborator

@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
Copy link
Owner Author

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
Copy link
Collaborator

ericman314 commented Apr 27, 2019

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
Copy link
Collaborator

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

@josdejong
Copy link
Owner Author

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
Copy link
Collaborator

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
Copy link

ajbouh commented May 20, 2019

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
Copy link
Collaborator

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

@josdejong
Copy link
Owner Author

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

@ajbouh
Copy link

ajbouh commented May 20, 2019

Thanks, I just added ericman314/UnitMath#13

@josdejong
Copy link
Owner Author

@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
Copy link
Collaborator

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
Copy link
Collaborator

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

@ajbouh
Copy link

ajbouh commented May 26, 2019 via email

@josdejong
Copy link
Owner Author

And how about something like this?

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

@ericman314
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Owner Author

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Owner Author

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Owner Author

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

@ericman314
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Owner Author

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
Copy link
Collaborator

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
Copy link
Owner Author

  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
Copy link
Collaborator

ericman314 commented Jun 3, 2019 via email

@josdejong
Copy link
Owner Author

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

😂 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
Copy link
Collaborator

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
Copy link
Owner Author

  • 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
Copy link
Collaborator

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

@josdejong
Copy link
Owner Author

Ok cool 👍

@josdejong josdejong mentioned this issue Jun 5, 2019
3 tasks
@josdejong josdejong mentioned this issue May 7, 2020
2 tasks
@jwir3
Copy link

jwir3 commented Jun 11, 2021

Any update on this? I'm using mathjs specifically for the unit functionality, and would love to just use a separate library, if possible.

@m93a
Copy link
Collaborator

m93a commented Jun 11, 2021

Hey @jwir3!
The standalone library for units indeed does exist – it's called UnitMath. It hasn't reached stable yet, however you can still get it from npm and check if it fits your needs. As far as I know, the library is currently in the middle of a big refactor, which will be released when it's done™. I guess @ericman314 will be able to provide you with more details :)

@ericman314
Copy link
Collaborator

Thanks @m93a that's exactly right. I had hoped to have v1 of UnitMath released long ago but extenuating circumstances prevented this. You can use the current version now, but some of the API may change. Specifically we're trying to simplify how users will configure and customize the library. So if you're just using the built-in units, and not adding any of your own, that functionality probably will not change much.

@ericman314
Copy link
Collaborator

UnitMath is now v1.0.0-rc.1, feel free to give it a try. I would love some feedback before we lock in the API at v1.0.0. https://www.npmjs.com/package/unitmath/v/1.0.0-rc.1

Repository owner locked and limited conversation to collaborators Sep 2, 2022
@josdejong josdejong converted this issue into discussion #2739 Sep 2, 2022

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Projects
None yet
Development

No branches or pull requests

6 participants