Expression parser broken for Python objects

Here's the place for discussion related to coding in FreeCAD, C++ or Python. Design, interfaces and structures.
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
User avatar
mfro
Posts: 663
Joined: Sat Sep 23, 2017 8:15 am

Re: Expression parser broken for Python objects

Post by mfro »

uwestoehr wrote: Mon Aug 15, 2022 2:42 pm In my opinion a proper solution would be that the expression is evaluated as it is already. If the result is unitless, the the unit of the QuantitySpinBox is added to the result. This way the user does not need to know our internals. He knows the unit for pressure is Pa and whatever he enter is then in Pa.
Then you would probably end up evaluating expressions differently depending on if they originate from a GUI element or not?
Cheers,
Markus
User avatar
uwestoehr
Veteran
Posts: 4961
Joined: Sun Jan 27, 2019 3:21 am
Location: Germany
Contact:

Re: Expression parser broken for Python objects

Post by uwestoehr »

wmayer wrote: Mon Aug 15, 2022 12:06 pm
The dialog tells the user the user also that the unit is "V". So as user I expect when I enter a "1" the result will be "1 V".
And if the original value was 1 mV and you entered the expression 1 + 2 do you then expect 3mV or 3V?
Now I got what you meant. I expect 3V because the unit for the input is V. So when I set for an expression e.g. 3e-3, I get 0.003 V. It is OK that the display shows me in this case 3 mV. Both values are the same.
The problem is that currently you enter "3" and get as result 3e-6 V.
User avatar
uwestoehr
Veteran
Posts: 4961
Joined: Sun Jan 27, 2019 3:21 am
Location: Germany
Contact:

Re: Expression parser broken for Python objects

Post by uwestoehr »

mfro wrote: Mon Aug 15, 2022 3:11 pm Then you would probably end up evaluating expressions differently depending on if they originate from a GUI element or not?
No, the issue is independent if you set the expression via the property editor or a GUI dialog.
wmayer
Founder
Posts: 20243
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Expression parser broken for Python objects

Post by wmayer »

uwestoehr wrote:As you can see in my screencast the original value was "1 V" not "1 mv".
This doesn't answer my question. I asked if the original value was 1 mV (conjunctive)...
Now I got what you meant. I expect 3V because the unit for the input is V. So when I set for an expression e.g. 3e-3, I get 0.003 V. It is OK that the display shows me in this case 3 mV. Both values are the same.
But when you set a value 1 mV into a spin box, select the entire text and enter 3 then it will become 3 mV and not 3 V because the last valid unit string was mV. IMO, this is the most sensible behaviour.
If the result is unitless, the the unit of the QuantitySpinBox is added to the result. This way the user does not need to know our internals. He knows the unit for pressure is Pa and whatever he enter is then in Pa.
The problem is that this won't work. An expression is bound to a property and if can be directly assigned via Python without opening an input widget.
mfro wrote:Then you would probably end up evaluating expressions differently depending on if they originate from a GUI element or not?
Exactly! Then we would end up in a really big mess... Not to mention that we break any existing project file out there.
uwestoehr wrote:]No, the issue is independent if you set the expression via the property editor or a GUI dialog.
You forgot about Python. There is no additional information available about which what unit was used.
User avatar
uwestoehr
Veteran
Posts: 4961
Joined: Sun Jan 27, 2019 3:21 am
Location: Germany
Contact:

Re: Expression parser broken for Python objects

Post by uwestoehr »

wmayer wrote: Mon Aug 15, 2022 4:39 pm But when you set a value 1 mV into a spin box, select the entire text and enter 3 then it will become 3 mV and not 3 V because the last valid unit string was mV. IMO, this is the most sensible behaviour.
In my opinion not, because the unit is "V". When nothing is specified the unit of the property should be used.
wmayer wrote: Mon Aug 15, 2022 4:39 pm You forgot about Python. There is no additional information available about which what unit was used.
I don't understand. We know the unit because the property "Potential" is App::PropertyPotential and for this the unit is specified as V.
Therefore it doesn't matter if I set the value via Python or the Gui.
I tested this: When I set the property "Potential" of the eletrostatic portential via Python as "1" I get as result 0.001 mV. When I set in the constraint dialog an expression as "1", I get the same.

As it is, it is not user-friendly. Therefore again, what is wrong with my proposal, that when the user either via python or an expression sets a unitless value like just "1", the unit of the property is taken/appended?
chrisb
Veteran
Posts: 53928
Joined: Tue Mar 17, 2015 9:14 am

Re: Expression parser broken for Python objects

Post by chrisb »

Looking at the Units calculator: If I select the Unit category "ElectricPotential" I get indeed mV and not V. Doesn't this tell me that mV is the default voltage unit in the default mm/kg/s/degree unit system? If that is wrong, then it may need a new Unit system to not invalidate existing projects.
A Sketcher Lecture with in-depth information is available in English, auf Deutsch, en français, en español.
wmayer
Founder
Posts: 20243
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Expression parser broken for Python objects

Post by wmayer »

Looking at the Units calculator: If I select the Unit category "ElectricPotential" I get indeed mV and not V. Doesn't this tell me that mV is the default voltage unit in the default mm/kg/s/degree unit system?
No. What unit is displayed to the user depends on the value: https://github.com/FreeCAD/FreeCAD/blob ... S.cpp#L260
So, one could implement a system that always shows V independent of the value.
Btw, because mm is the unit for lengths the unit of the electric potential for the quantity of 1 will be µV.
If that is wrong, then it may need a new Unit system to not invalidate existing projects.
This won't help. In FreeCAD the whole unit system is a pure (G)UI thing and all values that are used on algorithmic level are considered to be in mm/kg/s
User avatar
adrianinsaval
Veteran
Posts: 5541
Joined: Thu Apr 05, 2018 5:15 pm

Re: Expression parser broken for Python objects

Post by adrianinsaval »

wmayer wrote: Mon Aug 15, 2022 12:24 pm Maybe it should be disallowed to write dimensionless expressions when a unit is required. Or at least inside the dialog a warning could be printed.
+1 sounds like the more sensible solution and it's also going to help with the people using other unit systems and getting unexpected results due to not putting units in their expressions.
User avatar
uwestoehr
Veteran
Posts: 4961
Joined: Sun Jan 27, 2019 3:21 am
Location: Germany
Contact:

Re: Expression parser broken for Python objects

Post by uwestoehr »

wmayer wrote: Mon Aug 15, 2022 12:24 pm Maybe it should be disallowed to write dimensionless expressions when a unit is required. Or at least inside the dialog a warning could be printed.
When my proposal is not applicable, then fine with me.
For me the focus is to improve the situation, no matter how it is technically done.

However, I asked today 2 colleagues and they had the same opinion as I. The UI suggests the value is in V, so they assumed that it is sufficient e.g. just "3" to get 3 V as result. They have so may software to deal with therefore everything should "just work". in this case they clearly stated it is a no-go for them. I asked about forcing them to enter "3V" but this would not only be a keystroke more but is error-prone. One might hit the wrong key, so e.g. enter "3C", then get an error and annoyed since one loose time to read the error message.

Thinking about this, maybe it was bad design by me to use a QuantitySpinBox at all. Obviously people like to just input a value, so a simple FloatSpinBox should do the job better. But one cannot bind an expression to this.

The python-based dialogs offer their special input fields. But they are unintuitive as well:
- take the pressure constraint from the FEM WB
- enter there "1" -> you get "1 Pa"
- enter there "10" -> you get "10 kPa", so suddenly your input is multiplied by 1000
carlopav
Veteran
Posts: 2062
Joined: Mon Dec 31, 2018 1:49 pm
Location: Venice, Italy

Re: Expression parser broken for Python objects

Post by carlopav »

For what it's worth i have exactly the same opinion as uwe colleagues.
(Btw It would also be nice to be able to force a certain unit on both input and display.)
follow my experiments on BIM modelling for architecture design
Post Reply