Skip to content

Commit

Permalink
Fix oppia#19428: Same unit types should be accepted in Number with un…
Browse files Browse the repository at this point in the history
…its interaction (oppia#19706)

* Fix oppia#19428: Same unit types should be accepted in Number with units interaction

* add test

* simplitfy logic

* update isEqual logic

* fix linter

* fix linter

* refactor code

* add tests

* add test for Units map

* add test for map

* update comment

* update comment

* update test

* fix lint

* remove duplicate unit

* remove hard-coded validUnits array

* remove hard-coded validUnits array

* add type

* add type

* fix units

* add types for mathjs

* update code to handle prefixes

* fix linter

* undo changes to combine-tests.spec.ts

* update test for prefix

* add test to verify compleness of mapping

* fix spelling mistake

* break down long line

* add more tests

* add sorting case

* add test

* fix currency prefix issue

* reuse function

* fix linter

* add unit

* refactor code

* fix linter

* add comment
  • Loading branch information
AFZL210 authored May 12, 2024
1 parent c4b2c48 commit 7ab61fb
Show file tree
Hide file tree
Showing 8 changed files with 777 additions and 9 deletions.
42 changes: 42 additions & 0 deletions core/templates/domain/objects/NumberWithUnitsObjectFactory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,22 @@ import {Fraction} from 'domain/objects/fraction.model';
import {ObjectsDomainConstants} from 'domain/objects/objects-domain.constants';
import {Units, UnitsObjectFactory} from 'domain/objects/UnitsObjectFactory';
import {Unit, NumberWithUnitsAnswer} from 'interactions/answer-defs';
import {unit as mathjsUnit} from 'mathjs';

type CurrencyUnitsKeys = (keyof typeof ObjectsDomainConstants.CURRENCY_UNITS)[];

export const getCurrencyUnits = (): string[] => {
let currencyUnits: string[] = [];
for (const currency in ObjectsDomainConstants.CURRENCY_UNITS) {
const currencyInfo = ObjectsDomainConstants.CURRENCY_UNITS[currency];
currencyUnits.push(currency, ...currencyInfo.aliases);
}

return currencyUnits;
};

let currencyUnits = getCurrencyUnits();

/* Guidelines for adding new custom currency units in Number with Units
interaction:
Simply add currency unit to the dict of CURRENCY_UNITS constant and it will
Expand Down Expand Up @@ -123,6 +136,35 @@ export class NumberWithUnits {
units: this.units,
};
}

getCanonicalRepresentationOfUnits(): Unit[] {
const updatedUnits = this.units.map(({unit, exponent}: Unit) => {
const baseUnit = currencyUnits.includes(unit)
? ObjectsDomainConstants.UNIT_TO_NORMALIZED_UNIT_MAPPING[unit]
: mathjsUnit(unit).units[0].unit.name;

const unitPrefix = currencyUnits.includes(unit)
? ''
: mathjsUnit(unit).units[0].prefix.name;

let normalizedUnit =
(ObjectsDomainConstants.PREFIX_TO_NORMALIZED_PREFIX_MAPPING[
unitPrefix
] ?? '') +
ObjectsDomainConstants.UNIT_TO_NORMALIZED_UNIT_MAPPING[baseUnit];

return {
unit: normalizedUnit,
exponent: exponent,
};
});

updatedUnits.sort((a: Unit, b: Unit) => {
return a.unit.localeCompare(b.unit);
});

return updatedUnits;
}
}

@Injectable({
Expand Down
83 changes: 83 additions & 0 deletions core/templates/domain/objects/NumberWithUnitsObjectFactorySpec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -452,5 +452,88 @@ describe('NumberWithUnitsObjectFactory', () => {
).toThrowError('Number with type fraction cannot have a real part.');
}
);

it('should convert units to their canonical form', () => {
expect(
new NumberWithUnits(
'real',
1,
new Fraction(false, 0, 0, 1),
uof.fromRawInputString('celsius / meter')
).getCanonicalRepresentationOfUnits()
).toEqual(
new Units([
{unit: 'degC', exponent: 1},
{unit: 'm', exponent: -1},
]).units
);

expect(
new NumberWithUnits(
'real',
24,
new Fraction(false, 0, 0, 1),
uof.fromRawInputString('dollar / megatonne')
).getCanonicalRepresentationOfUnits()
).toEqual(
new Units([
{unit: 'dollar', exponent: 1},
{unit: 'Mton', exponent: -1},
]).units
);

expect(
new NumberWithUnits(
'real',
1,
new Fraction(false, 0, 0, 1),
uof.fromRawInputString('m / in')
).getCanonicalRepresentationOfUnits()
).toEqual(
new Units([
{unit: 'in', exponent: -1},
{unit: 'm', exponent: 1},
]).units
);

expect(
new NumberWithUnits(
'real',
1,
new Fraction(false, 0, 0, 1),
uof.fromRawInputString('m / Rupee')
).getCanonicalRepresentationOfUnits()
).toEqual(
new Units([
{unit: 'm', exponent: 1},
{unit: 'Rs', exponent: -1},
]).units
);
});

it('should convert units to their canonical form and sort them lexographically', () => {
expect(
new NumberWithUnits(
'real',
1,
new Fraction(false, 0, 0, 1),
new Units([
{unit: 'newton', exponent: 1},
{unit: 'meter', exponent: -1},
{unit: 'celsius', exponent: -2},
{unit: 'arcminutes', exponent: -3},
{unit: 'cycle', exponent: -1},
])
).getCanonicalRepresentationOfUnits()
).toEqual(
new Units([
{unit: 'arcmin', exponent: -3},
{unit: 'cycle', exponent: -1},
{unit: 'degC', exponent: -2},
{unit: 'm', exponent: -1},
{unit: 'N', exponent: 1},
]).units
);
});
});
});
14 changes: 14 additions & 0 deletions core/templates/domain/objects/objects-domain.constants.ajs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,20 @@ angular
.module('oppia')
.constant('CURRENCY_UNITS', ObjectsDomainConstants.CURRENCY_UNITS);

angular
.module('oppia')
.constant(
'UNIT_TO_NORMALIZED_UNIT_MAPPING',
ObjectsDomainConstants.UNIT_TO_NORMALIZED_UNIT_MAPPING
);

angular
.module('oppia')
.constant(
'PREFIX_TO_NORMALIZED_PREFIX_MAPPING',
ObjectsDomainConstants.PREFIX_TO_NORMALIZED_PREFIX_MAPPING
);

angular
.module('oppia')
.constant(
Expand Down
140 changes: 140 additions & 0 deletions core/templates/domain/objects/objects-domain.constants.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
// Copyright 2024 The Oppia Authors. All Rights Reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS-IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

/**
* @fileoverview unit tests for the units object domain constants.
*/

import {all, create} from 'mathjs';
import {getCurrencyUnits} from './NumberWithUnitsObjectFactory';
import {ObjectsDomainConstants} from './objects-domain.constants';

describe('ObjectsDomainConstants', () => {
let unitPrefixes: string[] = [];
let currencyUnits: string[] = [];
let mathjsUnits: string[] = [];
const math = create(all);

const isValidUnit = (unit: string): boolean => {
return currencyUnits.includes(unit) || math.Unit.isValuelessUnit(unit);
};

const isValidPrefix = (prefix: string): boolean => {
return unitPrefixes.includes(prefix);
};

const getUnitPrefixes = (): string[] => {
const prefixes = math.Unit.PREFIXES;
let prefixSet = new Set<string>();
for (const name in prefixes) {
// Skip if prefix type is 'NONE'.
if (name === 'NONE') {
continue;
}
for (const prefix in prefixes[name]) {
// Each prefix type has an empty key that we can ignore.
if (prefix === '') {
continue;
}
prefixSet.add(prefix);
}
}

return Array.from(prefixSet);
};

const getAllMathjsUnits = (): string[] => {
return Object.keys(math.Unit.UNITS);
};

beforeEach(() => {
unitPrefixes = getUnitPrefixes();
currencyUnits = getCurrencyUnits();
mathjsUnits = getAllMathjsUnits();
});

it(
'should check that every value in UNIT_TO_' +
'NORMALIZED_UNIT_MAPPING is a valid unit',
() => {
Object.values(
ObjectsDomainConstants.UNIT_TO_NORMALIZED_UNIT_MAPPING
).forEach(unit => {
expect(isValidUnit(unit)).toBe(true);
});
}
);

it(
'should check that every key in UNIT_TO_' +
'NORMALIZED_UNIT_MAPPING is a valid unit',
() => {
Object.keys(
ObjectsDomainConstants.UNIT_TO_NORMALIZED_UNIT_MAPPING
).forEach(unit => {
expect(isValidUnit(unit)).toBe(true);
});
}
);

it(
'should check that the UNIT_TO_NORMALIZED_UNIT_MAPPING contains' +
'all units supported by mathjs',
() => {
const units = Object.keys(
ObjectsDomainConstants.UNIT_TO_NORMALIZED_UNIT_MAPPING
)
.filter((unit: string) => {
return !currencyUnits.includes(unit);
})
.sort();
expect(units).toEqual(mathjsUnits.sort());
}
);

it(
'should check that every value in PREFIX_TO_' +
'NORMALIZED_PREFIX_MAPPING is a valid prefix',
() => {
Object.values(
ObjectsDomainConstants.PREFIX_TO_NORMALIZED_PREFIX_MAPPING
).forEach(prefix => {
expect(isValidPrefix(prefix)).toBe(true);
});
}
);

it(
'should check that every key in PREFIX_TO_' +
'NORMALIZED_PREFIX_MAPPING is a valid prefix',
() => {
Object.keys(
ObjectsDomainConstants.PREFIX_TO_NORMALIZED_PREFIX_MAPPING
).forEach(prefix => {
expect(isValidPrefix(prefix)).toBe(true);
});
}
);

it(
'should check that the PREFIX_TO_NORMALIZED_PREFIX_MAPPING contains' +
'all prefixes supported by mathjs',
() => {
const prefixes = Object.keys(
ObjectsDomainConstants.PREFIX_TO_NORMALIZED_PREFIX_MAPPING
).sort();
expect(prefixes).toEqual(unitPrefixes.sort());
}
);
});
Loading

0 comments on commit 7ab61fb

Please sign in to comment.