Handle UnitUtils.getDimensionName for mirek #4507

ccutrer · 2024-12-20T00:13:25Z

Fixes https://community.openhab.org/t/openhab-4-3-zigbee2mqtt-color-temperature-in-mired-not-working/160911/8

Signed-off-by: Cody Cutrer <[email protected]>

openhab-bot · 2024-12-20T00:17:23Z

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/openhab-4-3-zigbee2mqtt-color-temperature-in-mired-not-working/160911/13

andrewfg · 2024-12-20T10:28:51Z

bundles/org.openhab.core/src/main/java/org/openhab/core/types/util/UnitUtils.java

@@ -112,6 +113,10 @@ public class UnitUtils {
     * @return The Dimension string or null if the unit can not be found in any of the SystemOfUnits.
     */
    public static @Nullable String getDimensionName(Unit<?> unit) {
+        // Special cased, because the actual dimension is 1/Temperature


Hmm. Rather than making a special case here, I wonder if we should not try to make a 'proper' fix to systemUnit.isCompatible(unit) on line 137 below. ??

You're absolutely right - it should check if the inverse unit is compatible, just like QuantityType.toInvertibleUnit

} else if (compatibleDimension == null && (systemUnit.isCompatible(unit) || systemUnit.isCompatible(unit.inverse()))) { compatibleDimension = dimension; }

and Junit test..

@Test void testGetDimensionNameForInvertibleUnit() { assertEquals("Temperature", UnitUtils.getDimensionName(Units.MIRED)); assertEquals("Temperature", UnitUtils.getDimensionName(Units.KELVIN)); }

Signed-off-by: Cody Cutrer <[email protected]>

andrewfg · 2024-12-20T14:02:52Z

bundles/org.openhab.core/src/main/java/org/openhab/core/types/util/UnitUtils.java

@@ -134,7 +129,8 @@ public class UnitUtils {
                                LOGGER.warn("Unit field points to a null value: {}", field);
                            } else if (systemUnit.equals(unit)) {
                                return dimension;
-                            } else if (compatibleDimension == null && systemUnit.isCompatible(unit)) {
+                            } else if (compatibleDimension == null
+                                    && (systemUnit.isCompatible(unit) || systemUnit.inverse().isCompatible(unit))) {


Not 100% sure about this, but wondering if there there a functional difference between systemUnit.inverse().isCompatible(unit) and systemUnit.isCompatible(unit.inverse()) ??

I think the latter is probably more correct from a logic point of view...

andrewfg · 2024-12-20T14:06:36Z

bundles/org.openhab.core/src/test/java/org/openhab/core/types/util/UnitUtilsTest.java

@@ -120,6 +120,7 @@ public void testGetDimensionNameWithDimension() {

        unit = Objects.requireNonNull(UnitUtils.parseUnit("m"));
        assertThat(UnitUtils.getDimensionName(unit), is(Length.class.getSimpleName()));
+        assertThat(UnitUtils.getDimensionName(Units.MIRED), is(Temperature.class.getSimpleName()));


Perhaps add the following line to assert that nothing got broken due to the new inversion test..

assertThat(UnitUtils.getDimensionName(Units.KELVIN), is(Temperature.class.getSimpleName()));

Signed-off-by: Cody Cutrer <[email protected]>

andrewfg

LGTM

andrewfg · 2024-12-21T05:04:20Z

@ccutrer I was wondering if the code could be optimised by calling 'break' or 'return' to exit the for loops faster..

EDIT: umm .. probably not!

jimtng · 2024-12-22T14:53:09Z

Reminder to fix the typo in the PR title.

andrewfg · 2024-12-22T16:28:21Z

^ What typo?

Guessing that you meant the 'mirek' .. or?? FWIW there is no typo .. it stands for micro reciprocal Kelvin (which is the precise acronym for micro reciprocal degree (mired)..

jimtng · 2024-12-22T21:42:42Z

Thanks, it was my mistake. I didn't know.

ccutrer · 2024-12-23T13:26:39Z

@openhab/core-maintainers : can I get a review on this please. It needs picked to the 4.3 branch (see discussion on openhab/openhab-addons#17919)

holgerfriedrich

I cannot judge the side effects of accepting the inverse unit here. This might make sense for mired. But in general, I am not yet convinced if this is the correct approach:
I was thinking about ElectricConductance and ElectricResistence, where it seems wrong to do so. When I add a test for this, it is still fine, as both are a standard unit and seem not to reach this additional check.
Maybe other maintainers could step in here.

ccutrer · 2024-12-28T15:15:05Z

An inverse unit is raising to the -1 power (or flipping the numerator and denominator). It's not a logically opposite unit (electrical conductance and resistance are amps and ohms, which as far as units of measure are concerned, are completely unrelated).

The check in this PR is meant to match exactly the check in QuantityType.toInvertibleUnit (which we should probably removed in the 5.0 cycle, renaming to toUnit and removing the current toUnit - it was originally introduced as a separate method so as not to be a breaking change due to the change in return type).

andrewfg · 2024-12-28T17:15:31Z

electrical conductance and resistance are amps and ohms,

Wrong! Resistance is in Ohms and conductence is in Siemens. These are invertible units, and therefore mutually interconvertable. Just like Kelvin and Mirek are.

ccutrer · 2024-12-28T17:21:47Z

And yet again I'm wrong :).

From https://en.wikipedia.org/wiki/Siemens_(unit):

The conductance of a resistor with a resistance of five ohms, for example, is (5 Ω)−1, which is equal to a conductance of 200 mS.

So converting ohms to siemens makes sense (QuantityType.toUnit). But getDimensionName should probably prefer a unit's directly compatible dimension before checking for dimensions compatible with the inverse of the unit. I'll update to make two passes, and add additional tests.

…d first Signed-off-by: Cody Cutrer <[email protected]>

holgerfriedrich · 2024-12-28T18:27:22Z

I think it is fine for those two units mentioned above. I did some testing with this PR a week ago but did not come to a final conclusion..... Can it be that the new condition is not checked at all for those because the if before evaluates true?

andrewfg · 2024-12-28T18:36:24Z

the new condition is not checked at all for those because the if before evaluates true

Indeed. In Java the || operator always short circuits so the second term is only evaluated if the first returns false. (If you want to force evaluation of both terms without short circuiting, then use the | operator instead).

EDIT: but we do indeed need two loops -- the first to test all direct matches, and the second (if the first fails to find one) to find an inverse match. .. And probably (dare I say it) the proper logic I think would be put the iteration over the systems of units WITHIN these two loops, rather than wrapping around them .. because I think otherwise IMPERIAL.FAHRENHEIT might not find a match for METRIC.MIREK .. or??

Just an observation, but I suspect that these loops could become a classic exercise for a Java stream / flatmap / findFirst / orElse construct..

andrewfg · 2024-12-28T18:59:24Z

bundles/org.openhab.core/src/main/java/org/openhab/core/types/util/UnitUtils.java

@@ -112,6 +112,17 @@ public class UnitUtils {
     * @return The Dimension string or null if the unit can not be found in any of the SystemOfUnits.
     */
    public static @Nullable String getDimensionName(Unit<?> unit) {
+        String compatibleDimension = getDirectDimensionName(unit);
+        if (compatibleDimension == null) {
+            compatibleDimension = getDimensionName(unit.inverse());


Does unit.inverse() always return a non null value?

Yes. It may not make immediate logical sense (what's Meter^-1?), but it always has a value.

holgerfriedrich · 2024-12-28T19:09:13Z

          } else if (systemUnit.equals(unit)) {
              return dimension;
          } else if (compatibleDimension == null
                  && (systemUnit.isCompatible(unit) || systemUnit.isCompatible(unit.inverse()))) {
              compatibleDimension = dimension;
          }

@andrewfg I was referring to this snippet. If systemUnit.equals(unit) is true, the dimension is returned immediately.
When checking ElectricConductance, the first check hists and returns ElectricConductance immediately.
When checking ElectricResistance, the first check (systemUnit.equals(unit)) is false, compatibleDimension is set to ElectricResistance (but not returned). In the next step, it is compared to ElectricResistance (which returns immediately, thus the value in compatibleDimensions is not relevant).
Correct result - but only as the condition systemUnit.equals(unit) is fulfilled.

But I think the approach implemented now (i.e. first trying all compatible system units and compatible dimensions, and only using the inverse if nothing is found) is better, as it also covers the case that systemUnit.equals(unit) is not fulfilled correcty.

@ccutrer
But overall - does this make sense to say inverse units are compatible with the unit itself? I am still struggling to understand the reason behind.

andrewfg · 2024-12-28T20:29:10Z

does this make sense to say inverse units are compatible with the unit itself

Yes. The use case is that a binding may produce/consume values in a given unit and the UI may present those values in a respective inverted unit.

mherwege · 2024-12-28T20:32:58Z

But overall - does this make sense to say inverse units are compatible with the unit itself? I am still struggling to understand the reason behind.

I share your concern about this. I actually do not even agree Kelvin and Mired, both expressing color temperature are of the same dimension. It is an inverse (or reciprocal) dimension. And it is not because there is a conversion between them without another unit involved, they are of the same dimension.

I asked ChatGPT and here is the answer:

No, Kelvin (K) and Mired (M) are not of the same dimension, even though they are related. Here's why:

Kelvin (K)
Dimension: [Θ], representing temperature.
Kelvin directly measures the thermal energy of a black-body radiator.
Mired (M)
Mired is defined as: M = 10^6/T where T is the temperature in Kelvin (K).
Since it is the reciprocal of temperature (scaled by 10^6), its dimension becomes: [Θ]^-1. Hence, Mired represents the reciprocal of temperature.

The two units are thus not dimensionally equivalent. Mired quantifies something fundamentally different — the reciprocal nature of the temperature.
Key Difference:
Kelvin and Mired are related via a mathematical relationship, but their dimensions differ. Kelvin measures temperature directly, while Mired measures how "reciprocal" the temperature is, especially for practical adjustments in lighting or photography.

While practical, I fear we are on a slipery slope when we assume all reciprocal units are of the same dimension while they are not. Another such example is s vs Hz, time vs frequency. These are reciprocal as well.
By the way, cm^-1 seems to be a common unit in optics, so the inverse of m does exist.

andrewfg · 2024-12-28T21:12:46Z

Look guys 500 Mirek is IDENTICAL to 2000 K and 5 Ohm is IDENTICAL to 200 mSiemens. It is perfectly and totally logical to convert between those units.

ccutrer · 2024-12-28T21:27:02Z

So... let me reset here. The current state of the world in 4.3.0 is that quantities of reciprocal units are convertible to each other (as long as the caller is using QuantityType.toInvertibleUnit instead of QuantityType.toUnit, which all of core and most bindings -- especially those dealing with color temperature -- are). This helper method is used by some bindings to create channels of the proper dimension, when given a particular unit. In 4.3.0, this helper method returns null for MIRED. Therefore, channels created by bindings using this helper method for mireks are Number, not Number:Dimension, and cannot handle conversions between kelvin and mireks (meaning if a channel is in mireks, but can't tell the framework about this, then it cannot be used in the ColortemperaturePicker Basic UI control, or any user rules or custom UI elements that would send kelvin instead of mireks). An absolute minimal fix for this for 4.3.x would be to special case MIRED, and return "Temperature" in this method (my original commit in this PR). As this PR currently stands also seems reasonable to me to pick to 4.3.x, since there's not any known issues right now (a unit that doesn't have a named dimension, but whose reciprocal does have a named dimension, and that named dimension doesn't make sense).

For the future in 5.0, I see several directions we could go.

This PR as it currently stands: if a binding needs to create a dimensioned channel given only a unit, we need a dimension, and first choose the named dimension for unit, and if one does not exist, look for a named dimension for the reciprocal of the unit. QuantityType.toUnit should be deprecated/removed, or simply replaced with QuantityType.toInvertibleUnit.
Scale back this PR to just special case MIRED being a "Temperature". No other useful use case for reciprocal dimensions is currently known, so no need to be general with it. Perhaps this also means that bindings should only use QuantityType.toInvertibleUnit when they know they're possibly dealing with a (color) Temperature. Or maybe not... maybe I really do want an Item to hold ohms, even though the device is reporting in siemens.
Add a new named dimension "CCT" or "Correlated Color Temperature" or "Color Temperature" that mirek falls into. For the most part, it actually doesn't matter what the dimension of an item is, it's its unit that matters. There are just pieces of code that still check if a number item has any dimension (like in CommunicationManager), and then use QuantityType. An exception I can quickly find is a GroupItem with an aggregation function checks the compatibility of dimensions of all member items. This does seem inconsistent that a binding that provides a Kelvin channel would be a Number:Temperature, but a binding that provides a mirek channel would be a Number:CCT. Note that there are plenty of use cases for Kelvin that are normal temperatures, and not just CCT, so we would not want to special case it to be CCT.
Remove the concept of dimensioned number items entirely, instead relying on the unit of an item. While this seems fine for items and channels that always explicitly name the unit (and I believe all new items created in MainUI for dimensioned channels will have a unit explicitly defined), it ignores that many items do not have a unit defined, and instead infer the unit based on the dimension and the measurement system configured. In fact, due to unit coming from metadata, the unit of an item will always be inferred until its metadata is loaded. There are also likely still some bindings that don't deal with QuantityType, so those would need to be fixed. This would be nice to get rid of even more "fixup" code in CommunicationManager, but it's highly unlikely that we could be sure we caught every single binding that doesn't handle QuantityType.

mherwege · 2024-12-28T21:28:59Z

Look guys 500 Mirek is IDENTICAL to 2000 K and 5 Ohm is IDENTICAL to 200 mSiemens. It is perfectly and totally logical to convert between those units.

Equivalent is the right word, not identical. And I understand why it has been done, but I am not happy when it leads to generalizing this. We should have created something like a reciprocal dimension relationship and used that for these equivalencies, instead of calling it the same dimension. Because Ohm and mSiemens are units of an equivalent dimension, but not the same dimension. And to convert between them, you need a factor which is different by dimension.

andrewfg · 2024-12-28T21:38:41Z

No IDENTICAL I insist!

andrewfg · 2024-12-28T21:49:47Z

to convert between them, you need a factor which is different by dimension.

And that statement is totally irrelevant. To convert between Fahrenheit and Celsius or Feet and Metres also needs different calculations. But they are still IDENTICAL!

holgerfriedrich · 2024-12-28T22:25:54Z

Thanks, @ccutrer, for the comprehensive summary.
I see the technical needs. We need to find a suitable way to deal with different representations of color temperature.

Maybe a slightly different formulation of the problem is helpful. We do not necessarily require to obtain correct physical dimension for a given unit, but the dimension used by openHAB to represent data with that given unit. Prerequisite is that values can be converted between those units. For MIRED, the natural representation would be Temperature.

I don't have a strong opinion how this should look like (basically if this leads to renaming the methods or just adding the appropriate documentation). My personal feeling is that we could go for solution 2 you described above: just the special case for MIRED, and extending the method docs.

mherwege · 2024-12-28T22:28:31Z

With that, you should be able to convert losless between seconds (dimension Time) and Herz (dimension Time^-1, i.e. Frequency), just by defining a conversion factor. It doesn't generalize.

Look, I am perfectly fine with the solution for color temperature. I don't think it is ideal, but it works. But I don't think we should assume everything works that way for all dimensions. And assuming it does now, and then having to create exception upon exception is not what I call the right thing to do. That's why I regret having missed the opportunity to define reciprocal dimension relationships and conversions when they apply, and just call it the same dimension, which is not. They can represent the same thing (for color temperature), but it doesn't always, so it doesn't generalize.

That's the last I will say about this as this is very much a theoretical discussion at this point.

andrewfg · 2024-12-28T23:21:22Z

Ok. So if you want to go that way, then you probably should not kludge the getDimension() method, but rather revert it to its previous functionality i.e. close this OH core PR.

And therefore you need to add code in your specific addons PR to decide the special case of Mirek at the addon level, rather than in core. See openhab/openhab-addons#17929 (comment)

andrewfg · 2024-12-28T23:58:50Z

convert losless between seconds (dimension Time) and Herz (dimension Time^-1, i.e. Frequency), just by defining a conversion factor. It doesn't generalize

Umm. Yes indeed. IMHO a resonator with a frequency of 1 Hertz does indeed have a period of 1 second.

holgerfriedrich · 2024-12-29T11:41:26Z

If it helps in the bindings / UI, I would accept a special case for MIRED.
I think it does not generalize to other units, so we should avoid to check for all inverted units to avoid unwanted side effects.

To process data in MIRED, the bindings may need to be touched anyway. For example, if QuantityType toUnit("K") will return null for MIRED. See also the comment by @ccutrer regarding toUnit and toInvertibleUnit.

andrewfg · 2024-12-29T11:55:35Z

I would accept a special case for MIRED.

@holgerfriedrich @mherwege you guys insisted that the OH Core code should be done properly or not at all. You were worried about opening a can of worms of possible endless exceptions to your pure-dimensions-only rule. So adding Mirek now as an exception seems to be a complete cop out and no doubt the thin edge of the wedge. => If you insist on total purity in OH Core then you should NOT allow this exception in OH core. You should instead push the exception processing out into the addons. (Or devise a proper solution inside OH Core that does comply with your pure-dimensions-only rule). The current proposal is (in your universe) a kludge.

EDIT BTW the solution for @ccutrer 's case is here

jlaur · 2024-12-29T14:28:13Z

It seems like the topic covered by this PR might have both a short-term and a long-term goal. Obviously, something like this (as an example):

Remove the concept of dimensioned number items entirely

needs to be discussed further, and is out of scope for a 4.3.x patch release.

Short-term, what is the MVP we need to provide? Which bindings still have unresolved regressions in 4.3.x, i.e. do we need to fix multiple bindings, or only MQTT?

If only MQTT, would @andrewfg's proposition here be good enough for a quick fix in 4.3? The advantage would be that the fix would be isolated to MQTT and we wouldn't risk further regressions in core.

It seems there is still something to be discussed for a proper long-term solution, but the reason I'm focused on 4.3 right now is that a patch release is currently being planned (perhaps even for tomorrow), and it would be good to have a fix for regressions introduced in 4.3.

ccutrer · 2024-12-29T16:37:47Z

You should instead push the exception processing out into the addons.

Indeed. In fact, the JRuby helper library already has such an exception, and has for some time.

andrewfg · 2024-12-29T21:02:56Z

So @holgerfriedrich / @mherwege can one of you please close this PR as 'won't do'..

mherwege · 2024-12-29T22:28:41Z

@andrewfg I am not a maintainer, so am not able to do so.

special case UnitUtils.getDimensionName for mirek

b29673b

Signed-off-by: Cody Cutrer <[email protected]>

ccutrer requested a review from a team as a code owner December 20, 2024 00:13

This was referenced Dec 20, 2024

[mqtt.generic] Create the proper item type for channels with units openhab/openhab-addons#17929

Merged

[amazonechocontrol] Support QuantityType Color Temperature command openhab/openhab-addons#17919

Merged

andrewfg reviewed Dec 20, 2024

View reviewed changes

check if the inverse unit is compatible, instead of special case

0a70b77

Signed-off-by: Cody Cutrer <[email protected]>

ccutrer changed the title ~~special case UnitUtils.getDimensionName for mirek~~ handle UnitUtils.getDimensionName for mirek Dec 20, 2024

ccutrer changed the title ~~handle UnitUtils.getDimensionName for mirek~~ Handle UnitUtils.getDimensionName for mirek Dec 20, 2024

andrewfg reviewed Dec 20, 2024

View reviewed changes

andrewfg suggested changes Dec 20, 2024

View reviewed changes

address review comments

fcc4b3f

Signed-off-by: Cody Cutrer <[email protected]>

andrewfg approved these changes Dec 20, 2024

View reviewed changes

holgerfriedrich reviewed Dec 28, 2024

View reviewed changes

switch to two loops, ensuring a directly compatible dimension is foun…

ea48fdf

…d first Signed-off-by: Cody Cutrer <[email protected]>

andrewfg reviewed Dec 28, 2024

View reviewed changes

kaikreuzer requested a review from holgerfriedrich December 29, 2024 11:04

holgerfriedrich mentioned this pull request Dec 29, 2024

Are we sure that all bindings accept properly a value in Kelvin and a value in mirek as command? openhab/openhab-addons#17917

Closed

kaikreuzer added the won't fix label Dec 30, 2024

kaikreuzer closed this Dec 30, 2024

Handle UnitUtils.getDimensionName for mirek #4507

Handle UnitUtils.getDimensionName for mirek #4507

Conversation

ccutrer commented Dec 20, 2024 • edited Loading

openhab-bot commented Dec 20, 2024

andrewfg Dec 20, 2024

Choose a reason for hiding this comment

ccutrer Dec 20, 2024

Choose a reason for hiding this comment

andrewfg Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

andrewfg Dec 20, 2024

Choose a reason for hiding this comment

andrewfg Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

andrewfg Dec 20, 2024

Choose a reason for hiding this comment

andrewfg Dec 20, 2024

Choose a reason for hiding this comment

andrewfg left a comment

Choose a reason for hiding this comment

andrewfg commented Dec 21, 2024 • edited Loading

jimtng commented Dec 22, 2024

andrewfg commented Dec 22, 2024 • edited Loading

jimtng commented Dec 22, 2024

ccutrer commented Dec 23, 2024

holgerfriedrich left a comment

Choose a reason for hiding this comment

ccutrer commented Dec 28, 2024

andrewfg commented Dec 28, 2024 • edited Loading

ccutrer commented Dec 28, 2024

holgerfriedrich commented Dec 28, 2024

andrewfg commented Dec 28, 2024 • edited Loading

andrewfg Dec 28, 2024

Choose a reason for hiding this comment

ccutrer Dec 28, 2024

Choose a reason for hiding this comment

holgerfriedrich commented Dec 28, 2024 • edited Loading

andrewfg commented Dec 28, 2024

mherwege commented Dec 28, 2024 • edited Loading

andrewfg commented Dec 28, 2024

ccutrer commented Dec 28, 2024

mherwege commented Dec 28, 2024 • edited Loading

andrewfg commented Dec 28, 2024

andrewfg commented Dec 28, 2024

holgerfriedrich commented Dec 28, 2024

mherwege commented Dec 28, 2024

andrewfg commented Dec 28, 2024 • edited Loading

andrewfg commented Dec 28, 2024

holgerfriedrich commented Dec 29, 2024

andrewfg commented Dec 29, 2024 • edited Loading

jlaur commented Dec 29, 2024 • edited Loading

ccutrer commented Dec 29, 2024

andrewfg commented Dec 29, 2024

mherwege commented Dec 29, 2024

ccutrer commented Dec 20, 2024 •

edited

Loading

andrewfg Dec 20, 2024 •

edited

Loading

andrewfg Dec 20, 2024 •

edited

Loading

andrewfg commented Dec 21, 2024 •

edited

Loading

andrewfg commented Dec 22, 2024 •

edited

Loading

andrewfg commented Dec 28, 2024 •

edited

Loading

andrewfg commented Dec 28, 2024 •

edited

Loading

holgerfriedrich commented Dec 28, 2024 •

edited

Loading

mherwege commented Dec 28, 2024 •

edited

Loading

mherwege commented Dec 28, 2024 •

edited

Loading

andrewfg commented Dec 28, 2024 •

edited

Loading

andrewfg commented Dec 29, 2024 •

edited

Loading

jlaur commented Dec 29, 2024 •

edited

Loading