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

[blockly] Add common Math operations support for Quantity #2001

Closed
rkoshak opened this issue Aug 2, 2023 · 25 comments · Fixed by #2041
Closed

[blockly] Add common Math operations support for Quantity #2001

rkoshak opened this issue Aug 2, 2023 · 25 comments · Fixed by #2041
Labels
enhancement New feature or request main ui Main UI

Comments

@rkoshak
Copy link

rkoshak commented Aug 2, 2023

The problem

Associated with PR #2000.

Common mathematical operations provided by the standard JavaScript Math class cannot be used with Quantity objects.

Your suggestion

It probably doesn't make sense to support all of the possible operations but the following list makes sense to me.

  • abs
  • ceil
  • floor
  • max
  • min
  • maybe pow (I can go either way on this since raising a Quantity to a power likely changes the unit which obviously wouldn't actually happen here by implementing this outside of the QuantityType library)
  • maybe fround (I'm not sure I've ever seen this used in rules)
  • maybe trunc (also I'm not sure I've seen this used in rules and the same result can be achieved using ceil and floor I think

I can't imagine users needing to do trig or log match in rules with units so all that stuff can be skipped.

@stefan-hoehn
Copy link
Contributor

stefan-hoehn commented Aug 25, 2023

@rkoshak

  • ceil and floor are already available as part of the Round-block: Roundup = Ceil and Rounddown = Floor
  • trunc can be achieved with
image - not sure what **fround** is?

These are already working as well, including pow and abs and are backward compatible with the original Blockly-Math-Block

image

This here is a new min/max block that doesn't exist in the original math as the original one works with lists but I thought a minimal one with two inputs is actually pretty useful where the list one is rather seldomly used.

image

Note that there is a special case if one of the provided numbers is NOT a Quantity both inputs will be "downcasted" to number because I think we should not just detect the Unit of the other parameter make the number automatically a Quantity with that unit (see the last four blocks that will not return a quantity)

@stefan-hoehn
Copy link
Contributor

@rkoshak see my additional min/max block as well (I edited the above comment)

@rkoshak
Copy link
Author

rkoshak commented Aug 25, 2023

  • not sure what fround is?

Returns the nearest 32-bit single precision float representation of a number. It basically converts a double to a float if I'm interpreting it correctly.

The examples are interesting:

console.log(Math.fround(5.5));
// Expected output: 5.5

console.log(Math.fround(5.05));
// Expected output: 5.050000190734863

console.log(Math.fround(5));
// Expected output: 5

console.log(Math.fround(-5.05));
// Expected output: -5.050000190734863

It really isn't a standard round function and I suspect it's not something anyone would need to do in a rule unless they are parsing some modbus register or the like. Maybe it's not worth supporting.

Note that there is a special case if one of the provided numbers is NOT a Quantity both inputs will be "downcasted" to number because I think we should not just detect the Unit of the other parameter make the number automatically a Quantity with that unit (see the last four blocks that will not return a quantity)

I can see arguments for both ways. Recently I helped with a rule where someone needed to take a Temperature, add an offset to it, then divide by 10. The end result is still a Temperature. If you strip the units you'd have to add them back.

Then again, what unit is appropriate after all those operations? What unit do you get by dividing two temperatures? A ratio? So maybe stripping and adding the units back is appropriate as the whole set of operations were intended to correct a value received.

@mherwege
Copy link
Contributor

I can see arguments for both ways. Recently I helped with a rule where someone needed to take a Temperature, add an offset to it, then divide by 10. The end result is still a Temperature. If you strip the units you'd have to add them back.

Then again, what unit is appropriate after all those operations? What unit do you get by dividing two temperatures? A ratio? So maybe stripping and adding the units back is appropriate as the whole set of operations were intended to correct a value received.

I would argue that any operation with QuantityTypes can give a predictable QuantityType. Dividing temperature by temperature would give dimensionless. If you go the path of stripping units, you also need a block to get the unit from an item state to be able to add it back at the end.
I would prefer not to automatically strip units in the min/max blocks. Force to have no units or compatible units in both arguments and avoid any interpretation is what I would do.

@stefan-hoehn
Copy link
Contributor

I am not talking about math operations like Multiplication or the like. The block that does that allows the second input to be a number (see https://www.openhab.org/docs/configuration/blockly/rules-blockly-uom.html#quantity-computation)

What I am questioning is what it means to find the minimum / maximum of an input being dimensionless with one that has a unit. IMHO that doesn't make sense as you "comparing" apples with pies. When comparing to Quantities (see oh_quantity_compare )this isn't allowed either.

@mherwege
Copy link
Contributor

What I am questioning is what it means to find the minimum / maximum of an input being dimensionless with one that has a unit. IMHO that doesn't make sense as you "comparing" apples with pies. When comparing to Quantities (see oh_quantity_compare )this isn't allowed either.

Fully agree. It should require compatible dimensions (not necessarily units as long as they are compatible). comparing miles/hour and kilometer/hour for instance should be allowed.

@stefan-hoehn
Copy link
Contributor

Checking the dimension could be something I may add later and note, that this doesn't work for variables as the latter work require a runtime check which bloats the code (which is against OH blockly design principles). However, I may add that as a later enhancement when I have an idea how to show that warning (currently there is nothing like a good way to provide warnings in the blockly editor except an "alert" that I could provide).

@rkoshak
Copy link
Author

rkoshak commented Aug 28, 2023

If you go the path of stripping units, you also need a block to get the unit from an item state to be able to add it back at the end.

That should already be supported with the qty [value] [unit] block.

Checking the dimension could be something I may add later and note

QuantityType throws an exception if the units are not compatible with a reasonable description of the problem. I'm not sure this needs to be handled at the Blockly level.

@mherwege
Copy link
Contributor

That should already be supported with the qty [value] [unit] block.

Not sure I understand this statement. How would I do this?

QuantityType throws an exception if the units are not compatible with a reasonable description of the problem. I'm not sure this needs to be handled at the Blockly level.

Agree, I think theJavascript error messages would be sufficient.

@rkoshak
Copy link
Author

rkoshak commented Aug 29, 2023

Not sure I understand this statement. How would I do this?

image

Having to convert the number to text is a little awkward here but it wouldn't let my dock the calculation to the first argument of the Qty block directly. I suppose I could have assigned it to a variable first.

@mherwege
Copy link
Contributor

Not sure I understand this statement. How would I do this?

image

Of course, but you are assuming the unit you get from the item is °F and not °C, and adding a fixed °F unit back. I know I can do that. But how do I know what the unit of 'get numeric state of' was in the first place? Your are forcing the rule to know the unit of the item, which shouldn't be necessary and can lead to error. What if I change my item unit from °F to °C. I now must remember to also update all rules having this type of construct.
Anyway, if math functions with QuantityTypes are feature complete, I don't think this would be needed anyway. And it is a lot cleaner not to need it. It's just we should not do any dropping of units in any match function unless explicitely asked in the code.

@rkoshak
Copy link
Author

rkoshak commented Aug 30, 2023

Which is something I can assume in my system because my locale settings default to °F and all my Items have unit metadata dictating °F. But if there is doubt for some reason what unit the Item has:

What if I change my item unit from °F to °C. I now must remember to also update all rules having this type of construct.

There are other implications there with persistence which should make this a pretty rare occurrence. Every change of unit results in different units being saved so your database needs adjusting too. Changing units is not something that can be done causally.

Regardless, while I was looking for something else I notice the following in the docs:

Note: It is possible to create a unit-less (without unit) Quantity, however there is no advantage over using a number instead.

#

So the following should work I would thing unless there is some reason the units themselves do not allow it:

image

Unfortunately, after trying that out it seems, at least for temperature, it's not possible to do unitless operations with temperatures.

2023-08-30 09:51:22.243 [ERROR] [internal.handler.ScriptActionHandler] - Script execution of rule with UID 'scratchpad' failed: javax.measure.UnconvertibleException: javax.measure.IncommensurableException: one is not compatible with K

But other operations work but I'm not so sure the answer makes sense. For example:

//console.log(Quantity('65 °F').add(Quantity('1'))); // generates an error
console.log(Quantity('65 °F').divide(Quantity('1')));
console.log(Quantity('65 °F').multiply(Quantity('1')));

console.log(Quantity('65 °F').add(Quantity('1 °F')));
console.log(Quantity('65 °F').divide(Quantity('1 °F')));
console.log(Quantity('65 °F').multiply(Quantity('1 °F')));

produces

2023-08-30 10:00:22.252 [INFO ] [nhab.automation.script.ui.scratchpad] - 36.11111111111111111111111111111111 K
2023-08-30 10:00:22.255 [INFO ] [nhab.automation.script.ui.scratchpad] - 36.11111111111111111111111111111111 K
2023-08-30 10:00:22.257 [INFO ] [nhab.automation.script.ui.scratchpad] - 66 °F
2023-08-30 10:00:22.258 [INFO ] [nhab.automation.script.ui.scratchpad] - 65
2023-08-30 10:00:22.260 [INFO ] [nhab.automation.script.ui.scratchpad] - 20.06172839506172839506172839506173 K²

The fact that it reverted to Kelvin doesn't bother me as much as the number itself doesn't make sense to me. Maybe there is some weird conversion taking place I don't understand?

If I convert them back to °F

//console.log(Quantity('65 °F').add(Quantity('1'))); error
console.log(Quantity('65 °F').divide(Quantity('1')).toUnit('°F'));
console.log(Quantity('65 °F').multiply(Quantity('1')).toUnit('°F'));

console.log(Quantity('65 °F').add(Quantity('1 °F')).toUnit('°F'));
console.log(Quantity('65 °F').divide(Quantity('1 °F')).toUnit('°F'));
console.log(Quantity('65 °F').multiply(Quantity('1 °F')).toUnit('°F'));
// console.log(Quantity('65 °F').multiply(Quantity('1 °F')).toUnit('°F²')); error

produces

2023-08-30 10:04:35.853 [INFO ] [nhab.automation.script.ui.scratchpad] - -394.67 °F
2023-08-30 10:04:35.855 [INFO ] [nhab.automation.script.ui.scratchpad] - -394.67 °F
2023-08-30 10:04:35.857 [INFO ] [nhab.automation.script.ui.scratchpad] - 66 °F
2023-08-30 10:04:35.858 [WARN ] [nhab.automation.script.ui.scratchpad] - Failed to convert 65 to unit °F.
2023-08-30 10:04:35.859 [INFO ] [nhab.automation.script.ui.scratchpad] - null
2023-08-30 10:04:35.861 [WARN ] [nhab.automation.script.ui.scratchpad] - Failed to convert 20.06172839506172839506172839506173 K² to unit °F.
2023-08-30 10:04:35.861 [INFO ] [nhab.automation.script.ui.scratchpad] - null

🤷

Maybe this makes sense to someone but it doesn't to me. I don't see a pattern that makes sense.

@mherwege
Copy link
Contributor

Unfortunately, after trying that out it seems, at least for temperature, it's not possible to do unitless operations with temperatures.

That doesn’t look good at all. Temperature with Fahrenheit is tricky as the sequence of operations has in impact due to it not being a strictly linear conversion. But I also don’t see logic in this. This is not related to this issue though and should get its own issue.
Does it have the same wrong behavior with a pure number?
It would be interesting to try with something like energy (kJ and kWh) as an example.

@rkoshak
Copy link
Author

rkoshak commented Aug 30, 2023

Much more reasonable with kWh.

Code

//console.log(Quantity('65 kWh').add(Quantity('1'))); error
console.log(Quantity('65 kWh').divide(Quantity('1')));
console.log(Quantity('65 kWh').multiply(Quantity('1')));

console.log(Quantity('65 kWh').add(Quantity('1 kWh')));
console.log(Quantity('65 kWh').divide(Quantity('1 kWh')));
console.log(Quantity('65 kWh').multiply(Quantity('1 kWh')));

Logs

2023-08-30 13:06:28.742 [INFO ] [nhab.automation.script.ui.scratchpad] - 65 kWh
2023-08-30 13:06:28.743 [INFO ] [nhab.automation.script.ui.scratchpad] - 65 kWh

2023-08-30 13:06:28.744 [INFO ] [nhab.automation.script.ui.scratchpad] - 66 kWh
2023-08-30 13:06:28.745 [INFO ] [nhab.automation.script.ui.scratchpad] - 65
2023-08-30 13:06:28.746 [INFO ] [nhab.automation.script.ui.scratchpad] - 65 kWh²

Again with kJ

//console.log(Quantity('65 kJ').add(Quantity('1')));
console.log(Quantity('65 kJ').divide(Quantity('1')));
console.log(Quantity('65 kJ').multiply(Quantity('1')));

console.log(Quantity('65 kJ').add(Quantity('1 kJ')));
console.log(Quantity('65 kJ').divide(Quantity('1 kJ')));
console.log(Quantity('65 kJ').multiply(Quantity('1 kJ')));
2023-08-30 13:09:29.492 [INFO ] [nhab.automation.script.ui.scratchpad] - 65 kJ
2023-08-30 13:09:29.494 [INFO ] [nhab.automation.script.ui.scratchpad] - 65 kJ

2023-08-30 13:09:29.495 [INFO ] [nhab.automation.script.ui.scratchpad] - 66 kJ
2023-08-30 13:09:29.496 [INFO ] [nhab.automation.script.ui.scratchpad] - 65
2023-08-30 13:09:29.498 [INFO ] [nhab.automation.script.ui.scratchpad] - 65 kJ²

It still doesn't like add if the constant doesn't have units with the "one is not compatible with..." error.

But the divide and multiply seems reasonable.

But from this I conclude that addition and subtraction without units is simply not supported for many (most? all?) quantity types. If that's the case, does it make sense to allow that in Blockly?

I see the following options:

  1. Don't allow comparisons and addition and subtraction with plain numbers. Both operands in these cases must be Quantity. Multiply and divide can support a plain number as an operand.
  2. If one operand is a plain number, drop the units from all the operands and return a plain number. If units need to be added back that's up to the user to do.
  3. Make the end user handle it. In this case, if the end user insists on doing operations with operands that don't have units it's up to them to drop the units and add them back. To do this well though I think we need a block that returns the current unit of a Quantity so we can avoid the rule needing to have knowledge about the units.
  4. If one operand doesn't have units, promote it to the same unit as the other operand for comparisons and addition and subtraction.

@mherwege
Copy link
Contributor

console.log(Quantity('65 °F').divide(Quantity('1')));

The result is interesting. 100 °F = 36.1111111 °C. It seems that number is interpreted as a delta of 36.11111111 K. How that happens? Beats me at the moment. I will probably have to run this through a debugger.

@mherwege
Copy link
Contributor

I see the following options:

  1. Don't allow comparisons and addition and subtraction with plain numbers. Both operands in these cases must be Quantity. Multiply and divide can support a plain number as an operand.
  2. If one operand is a plain number, drop the units from all the operands and return a plain number. If units need to be added back that's up to the user to do.
  3. Make the end user handle it. In this case, if the end user insists on doing operations with operands that don't have units it's up to them to drop the units and add them back. To do this well though I think we need a block that returns the current unit of a Quantity so we can avoid the rule needing to have knowledge about the units.
  4. If one operand doesn't have units, promote it to the same unit as the other operand for comparisons and addition and subtraction.

For me, 1 is the preferred option.

@rkoshak
Copy link
Author

rkoshak commented Aug 30, 2023

I think 1 is my preferred option as well.

@stefan-hoehn
Copy link
Contributor

Perfect, if I get you right, this is exactly what I have implemented.

@rkoshak
Copy link
Author

rkoshak commented Sep 5, 2023

I don't think so. If it's still the same as what you described above:

Note that there is a special case if one of the provided numbers is NOT a Quantity both inputs will be "downcasted" to number because I think we should not just detect the Unit of the other parameter make the number automatically a Quantity with that unit (see the last four blocks that will not return a quantity)

what you've implemented is 2, not 1. 1 would be to throw an error if one of the operands is a Quantity and the other is not for addition, subtraction, and comparison operations. Blockly should not need to do anything special in these cases though. The underlying library will generate the error.

@stefan-hoehn
Copy link
Contributor

Sorry for only getting back now as I was caught with other issues for quite a while and I wanted to look into this issue with enough time.

You are right. I was referring to 2. So this is what you would like to have implemented:

Don't allow comparisons and addition and subtraction with plain numbers. Both operands in these cases must be Quantity. Multiply and divide can support a plain number as an operand.

Regarding comparison
image

  • It currently supports the following types: 'oh_quantity', 'oh_itemtype', 'oh_item', 'String' (note that the last 3 expect a string that expresses a value with a unit)
  • the first one works
  • the second one throws an exception: Can not compare incompatible units.

Regarding math operations

image
  • You distinguish addition / subtraction with multiplication/division which makes sense
  • The block accepts on the first parameter the types: 'oh_quantity', 'oh_itemtype', 'oh_item'
  • The block accepts on the second parameter the types: oh_quantity', 'Number', 'oh_itemtype', 'oh_item' to allow multiplication and division with a scalar.
  • Blocky DOES NOT check if the scalar is used on addition or substraction, hence it will throw an exception: Argument of wrong type provided, required Item, string or Quantity

Regarding "math_single" like square, log etc..

  • this is not affected as it only takes one param

So the only new block that would be affected would be MIN / MAX

Regarding math min/max

image
  • It currently allows 'Number', 'oh_quantity' as type on both input params
  • this currently works and returns a 3 (as the room temp is higher) as I am doing some "magic" in the background that "upcasts" the number (on either param side) to the equal unit of the other input.

-> I am correct that you would expect it to throw an exception instead? So the only change would be to take out that "magic" for that one block, correct?

@mherwege
Copy link
Contributor

-> I am correct that you would expect it to throw an exception instead? So the only change would be to take out that "magic" for that one block, correct?

I would indeed prefer not to have any magic there. After all, the magic can also lead to unintended consequences. What if WZ_Temperatur is updated with different units? What unit would you take for 3? The system unit? The current unit of the item state? I prefer to be clear. That allows to take the maximum of a temperature in °C and °F for instance.

@stefan-hoehn
Copy link
Contributor

ok, I would the remove it from the code during the week and then hopefully the new blocks will be available soon.

@stefan-hoehn
Copy link
Contributor

@mherwege @rkoshak

I was finally able to implement this upon your request and it even tells the user about the issue in the editor before actually receiving the info only during runtime.

image

@mherwege
Copy link
Contributor

Great. I think this is the right appproach.

@stefan-hoehn
Copy link
Contributor

To add something: the fact that we expect both inputs to be the same actually allowed my to support variables better in this case. Unfortunately variable types cannot be detected via blockly, so I HAVE to assume the type of the variable. In the case of min/max having two vars, I do have to assume that these are numbers. However, when one of the inputs is not a variable I can now detect that type and base my code generation of the non-var-input-type which is nice.

florian-h05 pushed a commit that referenced this issue Oct 8, 2023
Closes #2001.

Adds Quantity support for more math blocks:
- math_single had to be reimplemented
- math_minmax was added

Note that there is a special case on min/max if the inputs are not of
equal type an error will be shown to user.

In the special case of variables Blockly does its best to detect the
right code to be generated for the min/max block:
- both are variables -> then numerical input is expected
- one of inputs is a variable: then blockly uses the type of the other
non-var-block to base the generation on (either number or quantity
comparison)
- note that no type conversion of the inputs is done

Also-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Stefan Höhn <mail@stefanhoehn.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request main ui Main UI
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants