Unit toSI() uses 'mm' instead of 'm' for small 'ft' values

[BjoernSimon]

I am using Math.js Units to parse a string value and always convert it to the SI system to normalize the values. This works very well except for small 'ft' length and 'ft^2' area values. I did not try it for other units as I am assuming this is a generic issue.

Issue 1:
Unit.parse('0.111 ft^2').toSI() converts into '10312.23744 mm^2'instead of '0.01031223744 m^2'

Issue 2:
getting the 'unit' from JSON immediately after constructing it e.g. via Unit.parse(value).toSI().toJSON().unit) returns 'm^2' (correct SI unit) instead of 'mm^2' (the unit it was incorrectly converted to, see issue 1).

Please see example code and output below for details.

const math = require('mathjs');  //v5.2.3
math.config({number: 'BigNumber'}); // to change to BigNumbers.
const Unit = math.type.Unit;

function mathjsUnitBug(value) {
    console.log("\nOriginal value:     ", value)
    const unitObj = Unit.parse(value).toSI(); //BigNumber

    const unitJson = unitObj.toJSON();
    console.log("unitJson.unit:      ", unitJson.unit)
    console.log("unitObj.toString(): ", unitObj.toString())
}

Results in

Original value:      11.1 ft^2
unitJson.unit:       m^2
unitObj.toString():  1.031223744 m^2

Original value:      0.111 ft^2
unitJson.unit:       m^2
unitObj.toString():  10312.23744 mm^2

Original value:      11.1 ft
unitJson.unit:       m
unitObj.toString():  3.38328 m

Original value:      0.111 ft
unitJson.unit:       m
unitObj.toString():  33.8328 mm

[josdejong]

I think how toSI() is implemented right now is that it converts units like feet and inch to meter, but it does still allow using prefixes like millimeters, right @ericman314?

[ericman314]

Jos, I'm not certain. I don't think the intent was to auto-select the best prefix. This could be a bug.

[josdejong]

Looking at the unit tests of .toSI() I think it was not intentional behavior and we can consider this a bug:

mathjs/test/type/unit/Unit.test.js

Lines 1240 to 1264 in 638ed7e

	describe('toSI', function () {
	it('should return a clone of the unit', function () {
	const u1 = Unit.parse('3 ft')
	const u2 = u1.toSI()
	assert.strictEqual(u1 === u2, false)
	})
	it('should return the unit in SI units', function () {
	assert.strictEqual(Unit.parse('3 ft').toSI().format(10), '0.9144 m')
	})
	it('should return SI units for valueless units', function () {
	assert.strictEqual(Unit.parse('ft/minute').toSI().toString(), 'm / s')
	})
	it('should return SI units for custom units defined from other units', function () {
	Unit.createUnit({ foo: '3 kW' }, { override: true })
	assert.strictEqual(Unit.parse('42 foo').toSI().toString(), '1.26e+5 (kg m^2) / s^3')
	})
	it('should throw if custom unit not defined from existing units', function () {
	Unit.createUnit({ baz: '' }, { override: true })
	assert.throws(function () { Unit.parse('10 baz').toSI() }, /Cannot express custom unit/)
	})
	})

[josdejong]

Anyone interested in helping to fix this bug?

[ericman314]

I should be able to get to it within a week or two.

[ericman314]

Okay it was easier than I thought.

On a similar note, I really dislike the side-effects in Unit.format, (or equivalently, Unit.toString) in which the units are simplified lazily. It always seems to break things. Also it is in that function that the best prefix is applied, meaning that unless fixPrefix is set, the prefix can change whenever toString is called on the unit. I'm open to suggestions. I feel like if we can separate the unit calculation logic from the formatting of the unit a little better, these problems might go away. Or at the very least remove the side-effects from format.

[harrysarson]

I really dislike the side-effects in Unit.format

I could not agree more

[josdejong]

@BjoernSimon this issue should be fixed now in v5.3.1 thanks to the fix provided by Eric.

@ericman314 would it work if we would create a method Unit.simplify() which returns a new, simplified Unit, and when calling Unit.format(), basically return this.simplify().format()? Or just do this internally in format: don't change the unit itself but create a temporary simplified unit?

[ericman314]

I think so. Perhaps the right way to approach this is to make Units immutable, and refactor all the methods as necessary. I imagine the API could change significantly so it might be a breaking change.

Edit: After looking at the existing functions, must units are cloned rather than mutated, so I think we could keep the API essentially the same.

[harrysarson]

Moving towards immutability is a good thing in my book 👍

[ericman314]

PR #1344 removes the side-effects.

[josdejong]

In the refactoring I'm working on (#71) I smashed quite some times into issues around mathjs functions not being immutable but for example listening for config changes, which makes it impossible to really isolate them and get tree-shaking working, see my comment here: #71 (comment). The refactoring really pushes us towards pure functions, which is great!

[josdejong]

I think we can close this issue, it was solved by Eric already.