Skip to content

Commit c3ee5ba

Browse files
josdejonggwhitney
andauthored
fix: Use matching type for one when operating with valueless unit and a number (#3454)
Previously, when a valueless unit multiplied/divided something with a numerical value, it would just use ordinary 1 of type `number` in place of its null value. Now it matches the type of one used with the other numerical value. Resolves #3450. --------- Co-authored-by: Glen Whitney <[email protected]>
1 parent 4e88b3b commit c3ee5ba

File tree

3 files changed

+39
-4
lines changed

3 files changed

+39
-4
lines changed

HISTORY.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@
22

33
# Unpublished changes since 14.4.0
44

5+
- Fix: #3450 support multiplication of valueless units by arbitrary types
6+
(#3454).
57
- Feat: increase performance of the `map` and `forEach` methods of
68
`DenseMatrix` (#3446). Thanks @dvd101x.
79

src/type/unit/Unit.js

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -657,8 +657,9 @@ export const createUnitClass = /* #__PURE__ */ factory(name, dependencies, ({
657657

658658
// If at least one operand has a value, then the result should also have a value
659659
if (this.value !== null || other.value !== null) {
660-
const valThis = this.value === null ? this._normalize(1) : this.value
661-
const valOther = other.value === null ? other._normalize(1) : other.value
660+
const valThis = this.value === null ? this._normalize(one(other.value)) : this.value
661+
const valOther = other.value === null ? other._normalize(one(this.value)) : other.value
662+
662663
res.value = multiplyScalar(valThis, valOther)
663664
} else {
664665
res.value = null
@@ -709,8 +710,8 @@ export const createUnitClass = /* #__PURE__ */ factory(name, dependencies, ({
709710

710711
// If at least one operand has a value, the result should have a value
711712
if (this.value !== null || other.value !== null) {
712-
const valThis = this.value === null ? this._normalize(1) : this.value
713-
const valOther = other.value === null ? other._normalize(1) : other.value
713+
const valThis = this.value === null ? this._normalize(one(other.value)) : this.value
714+
const valOther = other.value === null ? other._normalize(one(this.value)) : other.value
714715
res.value = divideScalar(valThis, valOther)
715716
} else {
716717
res.value = null
@@ -772,6 +773,22 @@ export const createUnitClass = /* #__PURE__ */ factory(name, dependencies, ({
772773
}
773774
}
774775

776+
/**
777+
* Create a value one with the numeric type of `typeOfValue`.
778+
* For example, `one(new BigNumber(3))` returns `BigNumber(1)`
779+
* @param {number | Fraction | BigNumber} typeOfValue
780+
* @returns {number | Fraction | BigNumber}
781+
*/
782+
function one (typeOfValue) {
783+
// TODO: this is a workaround to prevent the following BigNumber conversion error from throwing:
784+
// "TypeError: Cannot implicitly convert a number with >15 significant digits to BigNumber"
785+
// see https://github.com/josdejong/mathjs/issues/3450
786+
// https://github.com/josdejong/mathjs/pull/3375
787+
const convert = Unit._getNumberConverter(typeOf(typeOfValue))
788+
789+
return convert(1)
790+
}
791+
775792
/**
776793
* Calculate the absolute value of a unit
777794
* @memberof Unit

test/unit-tests/type/unit/Unit.test.js

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1098,6 +1098,22 @@ describe('Unit', function () {
10981098
assert.deepStrictEqual(unit4.divide(unit5), unit6)
10991099
})
11001100

1101+
it('should multiply valueless units by any supported numeric type', function () {
1102+
const valuelessF = new Unit(null, 'fahrenheit')
1103+
assert.strictEqual(valuelessF.multiply(math.bignumber(123)).format(12), '123 fahrenheit')
1104+
assert.strictEqual(valuelessF.multiply(math.fraction(123, 2)).format(12), '123/2 fahrenheit')
1105+
assert.strictEqual(valuelessF.multiply(math.complex(123, 123)).format(12), '(123 + 123i) fahrenheit')
1106+
assert.strictEqual(valuelessF.multiply(123).format(12), '123 fahrenheit')
1107+
})
1108+
1109+
it('should divide valueless units by any supported numeric type', function () {
1110+
const valuelessF = new Unit(null, 'fahrenheit')
1111+
assert.strictEqual(valuelessF.divide(math.bignumber(1).div(123)).format(12), '123 fahrenheit')
1112+
assert.strictEqual(valuelessF.divide(math.fraction(2, 123)).format(12), '123/2 fahrenheit')
1113+
assert.strictEqual(valuelessF.divide(math.complex(0.25, 0.25)).format(12), '(2 - 2i) fahrenheit')
1114+
assert.strictEqual(valuelessF.divide(1 / 123).format(12), '123 fahrenheit')
1115+
})
1116+
11011117
// eslint-disable-next-line mocha/no-skipped-tests
11021118
it.skip('should cancel units in numerator and denominator', function () {
11031119
assert.strictEqual(math.evaluate('2 J/K/g * 2 g').toString(), '4 J / K')

0 commit comments

Comments
 (0)