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

Should we use Quantity.Scale to prevent certain operations? #167

Closed
keilw opened this issue Jan 25, 2019 · 10 comments
Closed

Should we use Quantity.Scale to prevent certain operations? #167

keilw opened this issue Jan 25, 2019 · 10 comments

Comments

@keilw
Copy link
Member

keilw commented Jan 25, 2019

There was a question at OOP (@filipvanlaenen will remember) about preventing e.g. the multiplication of CELSIUS.
Is a certain Quantity.Scale able to control that, or would we need a LevelOfMeasurement enum after all? https://en.wikipedia.org/wiki/Level_of_measurement#Mathematical_operations tells, that multiplication is only allowed on values at RATIO level, so could e.g. RELATIVE also be used to throw an exception for methods like Quantity.multiply() or do we have to get the level back in all relevant places?

@keilw keilw added this to the 2.0 milestone Jan 25, 2019
@desruisseaux
Copy link
Contributor

Actually multiplication by CELSIUS can be allowed, depending on the context. It is allowed if a quantity in CELSIUS is a difference rather than an absolute measurement. Even if the quantity is an absolute measurement, the multiplication is still allowed if the quantity is converted to KELVIN before the multiplication. The proposal to throw an UnsupportedOperationException was (in my understanding) for avoiding to surprise the user with a conversion that he may not expect. But my point of view is that it would be replacing a potentially surprising behavior by another surprising behavior (exception throws under circumstances that may become difficult to identify if the multiplication appears in the middle of more complex operations).

@desruisseaux
Copy link
Contributor

Said otherwise: Wikipedia said that CELSIUS as an absolute measurement can not be multiplied as-is, but it does not said that we are not allowed to convert to something else (KELVIN) for enabling multiplication.

@keilw
Copy link
Member Author

keilw commented Jan 26, 2019

Then it seems if ABSOLUTE allows to prevent multiplications of e.g. CELSIUS that seems enough. Let's leave it open if others may have issues with that, but it would be nice to stick with just one enum for now.

@keilw
Copy link
Member Author

keilw commented Jan 28, 2019

There should be some throws or similar description in JavaDoc, like in Unit.root().
Having defined that enum in Quantity only, what about Unit.multiply(Unit)?
Is CELSIUS.multiply(CELSIUS) OK? I guess it might because the result would be another unit, but just to cover all operations.

https://www.wikihow.com/Convert-Between-Fahrenheit,-Celsius,-and-Kelvin mentions conversions that involve Quantity.multiply(1.8) as part of converting a CELSIUS temperature to FAHRENHEIT.
So if we do prevent it for any scale or other precondition it should be well-documented to avoid surprised or angry users.

@desruisseaux
Copy link
Contributor

I don't think we should throw exception. Quantity.multiply(Quantity) is a valid operation even if the quantities are absolute measurement in CELSIUS, providing that the quantities are converted to KELVIN before multiplication. The rule is that we can not multiply CELSIUS as-is. There is no rule saying that we can not multiply the value after conversion to a ratio unit.

@keilw
Copy link
Member Author

keilw commented Jan 28, 2019

So what is the preferred runtime behavior in this case?
Same as before? If so, then I guess we can close this ticket, if no action is required.

So far unitsofmeasurement/indriya#128 in the RI is the only issue that looks like it may use the scale.

@keilw keilw changed the title Can we use Quantity.Scale to prevent certain operations? Should we use Quantity.Scale to prevent certain operations? Jan 28, 2019
@desruisseaux
Copy link
Contributor

I don't know what is "same as before". The scale is used for determining how to convert to KELVIN before the multiplication:

  1. If absolute: convert 2°C as 275.15 K, then multiply.
  2. If difference: convert 2°C as 2 K, then multiply.

Implementation is free to convert the result back to °C if they wish. My proposal is to keep the result of 1 in kelvin for less surprising behavior.

@keilw
Copy link
Member Author

keilw commented Jan 28, 2019

Ok but it would not be prevented e.g. by throwing an ArithmeticException or similar.
That was asked by an OOP audience member. Unless we consider this crucial right now, I would rather not reintroduce the LevelOfMeasurement (which would help to prevent multiplication for some levels) at least not in 2.0. Unless there is a demand for this?

Btw. @desruisseaux could you add the last comment in the right form to unitsofmeasurement/indriya#128, I think it makes most sense there.
If no reason is found to prevent an operation altogether, then I would close this ticket soon.

@desruisseaux
Copy link
Contributor

I see no need for LevelOfMeasurement and no need to prevent multiplication for some levels. What is forbidden is doing multiplication without prior conversion for some levels.

@keilw keilw added the wontfix label Jan 28, 2019
@keilw
Copy link
Member Author

keilw commented Jan 28, 2019

Thanks, then let's close this with wontfix.

@keilw keilw closed this as completed Jan 28, 2019
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

2 participants