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

Incorrect unit conversion to 'decade' #1083

Closed
pedroteixeira opened this Issue Apr 15, 2018 · 5 comments

Comments

Projects
None yet
4 participants
@pedroteixeira

pedroteixeira commented Apr 15, 2018

in v4.1.1:

> mathjs.unit('20', 'year').to('decade').format()
'2 year

I think the expected output would be 2 decade, right?

I'm guessing some conflict with the deca prefix..

@harrysarson

This comment has been minimized.

Collaborator

harrysarson commented Apr 15, 2018

Confirming this behavour happens in the master branch.

> mathjs.unit(20, 'year').to('decade')
Unit {
  units: [ { unit: 
     { name: 'year',
       base: [Object],
       prefixes: [Object],
       value: 315576000,
       offset: 0,
       dimensions: [Array] },
    prefix: { name: '', value: 1, scientific: true },
    power: 1 } ],
  dimensions: [ 0, 0, 1, 0, 0, 0, 0, 0, 0 ],
  value: 631152000,
  fixPrefix: true,
  isUnitListSimplified: true }

Also mathjs.unit('20', 'year').to('decades').format() (note the s) fails with:

SyntaxError: Unit "decades" not found.

mathjs.unit('20', 'days').to('decade').format() -> '0.0054757015742642025 year'
mathjs.unit('20', 'days').to('decades').format() -> Error

@ericman314 ericman314 self-assigned this Apr 15, 2018

@ericman314 ericman314 added the bug label Apr 15, 2018

@ericman314

This comment has been minimized.

Collaborator

ericman314 commented Apr 15, 2018

This was a result of the decade unit having the wrong value ('year') for its name property.

I wrote a quick test that revealed there was one other typo: the unit watt had the name 'W'.

hertz has the name 'Hertz', but there's an alias that points from hertz to 'hertz' which resets the name of the hertz unit to 'hertz'. This seems unnecessary; maybe we can remove it and change the name to hertz?

The name property is redundant, we could easily generate the names from the keys.

@ericman314

This comment has been minimized.

Collaborator

ericman314 commented Apr 15, 2018

Should be resolved via #1084

@josdejong

This comment has been minimized.

Owner

josdejong commented Apr 17, 2018

The fix is merged, will be available in the first next release.

@josdejong

This comment has been minimized.

Owner

josdejong commented Apr 18, 2018

Fixed in v4.1.2

@josdejong josdejong closed this Apr 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment