Skip to content
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

"300 degC" wrongly converted to "300 K" by toSI() #3097

Closed
jkotan opened this issue Dec 4, 2023 · 6 comments
Closed

"300 degC" wrongly converted to "300 K" by toSI() #3097

jkotan opened this issue Dec 4, 2023 · 6 comments

Comments

@jkotan
Copy link

jkotan commented Dec 4, 2023

The problem is related to #2499 but it was not fixed by the corresponding PR.

When we convert units from degC to K in SciCat they are wrongly converted by toSI() i.e. without adding the offset (SciCatProject/scicat-backend-next#926 ).

In physics you never use Celsius to describe a difference of two various temperatures by always Kelvin
so during conversion from degC to K the offset should be always added.

You can workaround the problem using twice unit() , to() and toSi() but it looks complicated.

Is this behavour expected?

To Reproduce

>  const { unit, version } = await import("mathjs");

> version
'12.1.0'

>  unit(300. "degC").toSI().toJSON()
{ mathjs: 'Unit', value: 300, unit: 'K', fixPrefix: true }

>  unit(300, "degC").to(unit("degC").toSI()).toJSON()
{ mathjs: 'Unit', value: 573.15, unit: 'K', fixPrefix: true }
@josdejong
Copy link
Owner

Thanks for reporting. At first sight this indeed looks like a bug in Unit.toSI to me, this method does not contain any logic to recon with a temperature offset like Unit.to does.

Any thoughts on this @costerwi or @gwhitney?

@gwhitney
Copy link
Collaborator

gwhitney commented Dec 6, 2023

I mean the whole situation is super complicated by whether a given units quantity is a specific temperature, or a difference in temperatures. I can't keep straight in my head the convention we were heading to between what units to use for which; I think maybe it was that degC should only be used for specific temperatures, and K for either specific temperature or differences? If so, then yes, using the offset when converting should happen. So sounds like this is a bug. The real best way to handle these things would be to get the modularization with respect to a separate Unit package done, and then put the energy into fixing that Unit package... sorry not to be of more help.

@josdejong
Copy link
Owner

Thanks for your inputs Glen.

I think the issue here is that the behavior of toSI() and to() are not consistent:

math.unit("300 degC").toSI().toString()   // "300 K"
math.unit("300 degC").to("K").toString()  // "573.15 K" 

Maybe .toSI() is just not on par with .to(). That could be related to the changes in .to() that we made in #2501.

@costerwi
Copy link
Contributor

costerwi commented Dec 6, 2023

Note this result is consistent between the two methods:

math.unit("300 degC^-1").to("K^-1").toString()  // "300 K^-1"
math.unit("300 degC^-1").toSI().toString()   // "300 K^-1"

I prefer the behavior of the existing .to() method and I think the toSI() example in the subject of this issue looks like a bug. Maybe it can be rewritten to identify the correct SI unit and then convert to it using .to().

@josdejong
Copy link
Owner

Maybe it can be rewritten to identify the correct SI unit and then convert to it using .to().

Reusing that logic would be nice indeed.

@josdejong
Copy link
Owner

This issue has been fixed now in v12.3.0 via #3118.

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

No branches or pull requests

4 participants