The code looks perfectly fine to me and is needed to make sure the proper conversion is done.

I am wondering if this conversion to system unit always applies for reciprocal units. I only found one example where it doesn’t, but it is esoteric and I would’t expect it to be of any use in our context:
The reciprocal (invertible) unit of length would be m-1. In optics a unit dioptre would be used as a unit (dpt). But there is also a unit for wavenumber, cm-1 called Kaiser. This is old, and not really used nowadays. But if it were, conversion to system unit and then taking the inverse would not yield the proper result.
I couldn’t find other examples, so this may be perfectly fine in our environment. But I would hate to now do this and see an example pop up in the future that invalidates the assumption you make here now.

So in conclusion, if we were to have dioptre as a unit for reciprocal length in the system, the change you make would also fix it for this, always converting to meter before inverting. But I cannot be 100% sure something else pops up in the future. In these cases, there is no easy rule anymore, because you would have to convert to a specific unit different from the system unit before inverting.

@mherwege I did a trawl through the addons and found about 40 cases using toInvertibleUnit; most of them are color temperature cases; and as far as I can tell the others are calling toInvertibleUnit where in fact toUnit would work; in those latter cases the target is generally a non inverted unit, so the method code falls through to toUnit anyway. I added a unit test to check that fall through use case too.

Apropos Dioptre and Kaiser -- AFAICT these units are not defined in OH so any potential problem with Kaiser is perhaps moot??

Apropos actual usages of toInvertibleUnit -- there are 33 addons that call it; of these 28 do so for color temperature, 1 for time units, 1 for temperature units, and 3 for generic units (o.o.binding.mqtt, o.o.io.homekit, o.o.transform.basicprofiles).

So how would you like to proceed? My proposal is as follows..

1. Include JUnit tests for all known valid conversions to inverted units -- currently all flavors of color temperature, Ohm/Siemens, and Second/Hertz are covered => do you have any others to propose?
2. Include JUnit tests for some conversions to non inverted units, i.e. test that toInvertibleUnit falls through to toUnit
3. Extend the toInvertibleUnit JavaDoc with a note similar to your comment above.

@mherwege => WDYT?

@andrewfg Your proposal makes sense. But I still have a very uncomfortable feeling about using invertible units in general. I would prefer to try to eliminate the use wherever there is no real reason to use it, combined with your proposal here.

Here is an example in core where invertible units may lead to very strange results:

In all, I am OK with your solution, but I also want to make it very clear there are strong limitations to the use of in invertible units. And that should probably also be made clear in the javadoc.

I would very much like to understand the few cases where invertible units are used for pure temperature and time. Is it necessary in these cases? And be careful with the transform profiles. Any calculation is a potential problem.

Apropos Dioptre and Kaiser -- AFAICT these units are not defined in OH so any potential problem with Kaiser is perhaps moot??

Indeed, it is. And I don’t think Kaiser will ever be defined. I also stated these units are likely not relevant. But that was not my point. If we code logic now, and someone at some point needs a unit that would create this kind of problem, nobody will remember. We would be building exception on top of exception.

@mherwege all your points duly noted.

1. I will modify the JavaDoc to incorporate your concerns.
2. For the bindings that 'wrongly' use toInvertibleUnit (they are Insteon (time) and MercedesMe (temperature)): => Should we open issues to have them fixed?
3. Do you want to open an issue for the OH core code that you cited above? I don't know the use case. I guess it is for group items? Note: the code that you cited produces a sum based on the unit of the first iterand in the for loop. So in the color temperature case if the first iterand is in Kelvin the result will be a QuantityType of the sums in Kelvin. And ditto if the first iterand is in Mirek. At least the sum is consistent across the units of all iterands. It is worth noting that this code would also produce odd results in ANY case of iterand Items with mixed units (e.g. Kelvin, Fahrenheit, Celsius) -- not because of toInvertibleUnit per se but due to the offset nature of the zero points of temperature units in general. => So perhaps rather than changing the OH Core Code, it would be better to add a note to the OH documentation for such use cases?

You would actually have to provide the unit for the calculation as an argument to be able to have a predictable result.

Indeed. And as mentioned above, it applies to ALL calculations based on sets of items with mixed units. So to be clear this is not really an issue with toInvertibleUnit .. it is rather an issue with the 'sum' algorithm when the set includes items having a non zero point offset.

Indeed. And as mentioned above, it applies to ALL calculations based on sets of items with mixed units. So to be clear this is not really an issue with toInvertibleUnit .. it is rather an issue with the 'sum' algorithm when the set includes items having a non zero point offset.

That is not correct, as the calculation functions will be done on the absolute scale since #3792. So there is no unpredictability.

Looking at the code of the arithmetic group functions, it looks like items of another dimension than the first are excluded from the sum. So depending on the unit of the first item, you will have a different set of items used in the function if there are invertible units in it. It would probably be better to use the unit of the group base item, instead of the unit of the first item, and then allow the reciprocal unit in the calculation after conversion to the base unit. I notice in the history the use of toInvertibleUnit in that method has explicitly been added to cope with color temperature, but in my view it does not work at all at this time (and that is from reading code, not testing). Also it is not used in all methods, only a few.

.. be careful with the transform profiles. Any calculation is a potential problem.

The StateFilterProfile explicitly uses toInvertibleUnit in calculateMedian below, but it is notable that other calculation methods use QuantityType.add() for summation instead, so it is not obvious why this one method requires toInvertibleUnit.

General comment: this calculation use case looks to be similar to the calculation use case that you cite in OH Core, so we should try to follow similar logic in resolving any potential issues.

The good news: for the case of StateFilterProfile there is probably no problem. Reason is that calculation are done over a time series set of values for the SAME Item, and the Item will not flip its unit over that time series, so all calculations will remain self consistent over that series.

The bad news: I am beginning to think that there may be bigger issues in other cases that calculate over sets of QuantityType. I think it is not just a question of toInvertibleUnit, and specifically not something where this PR would have an impact.

https://github.com/openhab/openhab-addons/blob/0b3383b95ccb5035532a5d54b4b4e208348807b6/bundles/org.openhab.transform.basicprofiles/src/main/java/org/openhab/transform/basicprofiles/internal/profiles/StateFilterProfile.java#L604

It would probably be better to use the unit of the group base item, instead of the unit of the first item

Yes. That solution also works for invertible units since the target unit is always determined.

The StateFilterProfile explicitly uses toInvertibleUnit in calculateMedian below, but it is notable that other calculation methods use QuantityType.add() for summation instead, so it is not obvious why this one method requires toInvertibleUnit.

I don’t think it is any different from the other methods from that perspective, only it was added later.

EDIT: Actually, it is fine in this case. The median method converts to the base unit of the item for all values and finds the median value using a statistics function I actually wrote myself. It adds the base unit at the end in the result again. So this is fine, except the previous remark: reciprocal unit are not of the same dimension, so not considered at all, even if they are intended to be considered in the code.

So this is fine

Ok. Shall I close the issue? Or would you prefer to open a PR that replaces toInvertibleUnit (which works implicitly) by toUnit (which works explicitly)?

Ok. Shall I close the issue? Or would you prefer to open a PR that replaces toInvertibleUnit (which works implicitly) by toUnit (which works explicitly)?

Keep it for now. I am not sure what the best solution is though. I will comment in the issue.