[bug] There is a bug about quontities

Here's the place for discussion related to coding in FreeCAD, C++ or Python. Design, interfaces and structures.
Fat-Zer
Posts: 171
Joined: Thu Oct 30, 2014 10:38 pm

[bug] There is a bug about quontities

Postby Fat-Zer » Mon Feb 29, 2016 7:52 am

Currently if a quantity is compared against a double (e.g. Precision::Confusion()) it will likely results in an expression with message "The error message is: Quantity::operator <(): quantities need to have same unit to compare".

In the FreeCAD this can be triggered e.g. by this:
  1. Create a Pad
  2. Switch Type to anything
  3. Switch Type back to Length
So I propose to allow comparison to Quantities with no Units rather than tracking all such places (I'm not sure how much there are). What do you think?

Propoused fix:

Code: Select all

diff --git a/src/Base/Quantity.cpp b/src/Base/Quantity.cpp
index 4bb599c..fa22b4b 100644
--- a/src/Base/Quantity.cpp
+++ b/src/Base/Quantity.cpp
@@ -73,7 +73,7 @@ bool Quantity::operator ==(const Quantity& that) const

 bool Quantity::operator <(const Quantity& that) const
 {
-    if(this->_Unit != that._Unit)
+    if(!that._Unit.isEmpty() && this->_Unit != that._Unit)
         throw Base::Exception("Quantity::operator <(): quantities need to have same unit to compare");

     return (this->_Value < that._Value) ;
@@ -81,7 +81,7 @@ bool Quantity::operator <(const Quantity& that) const

 bool Quantity::operator >(const Quantity& that) const
 {
-    if(this->_Unit != that._Unit)
+    if(!that._Unit.isEmpty() && this->_Unit != that._Unit)
         throw Base::Exception("Quantity::operator >(): quantities need to have same unit to compare");

     return (this->_Value > that._Value) ;
@@ -99,7 +99,7 @@ Quantity Quantity::operator /(const Quantity &p) const

 Quantity Quantity::pow(const Quantity &p) const
 {
-    if(!p._Unit.isEmpty())
+    if(!p._Unit.isEmpty())
         throw Base::Exception("Quantity::pow(): exponent must not have a unit");
     return Quantity(
         std::pow(this->_Value, p._Value),
@@ -109,13 +109,13 @@ Quantity Quantity::pow(const Quantity &p) const

 Quantity Quantity::operator +(const Quantity &p) const
 {
-    if(this->_Unit != p._Unit)
+    if(p._Unit.isEmpty() && this->_Unit != p._Unit)
         throw Base::Exception("Quantity::operator +(): Unit mismatch in plus operation");
     return Quantity(this->_Value + p._Value,this->_Unit);
 }
 Quantity Quantity::operator -(const Quantity &p) const
 {
-    if(this->_Unit != p._Unit)
+    if(p._Unit.isEmpty() && this->_Unit != p._Unit)
         throw Base::Exception("Quantity::operator +(): Unit mismatch in minus operation");
     return Quantity(this->_Value - p._Value,this->_Unit);
 }
I'll feel in a bugreport a bit later...
wmayer
Site Admin
Posts: 17247
Joined: Thu Feb 19, 2009 10:32 am

Re: [bug] There is a bug about quontities

Postby wmayer » Mon Feb 29, 2016 12:53 pm

So I propose to allow comparison to Quantities with no Units rather than tracking all such places (I'm not sure how much there are). What do you think?
IMO, we should be rather strict on this and force client code to make sure that units do match. To find all cases where quantities with doubles is compared can be easily detected by using the keyword "explicit" in front of the constructor of Quantity

Code: Select all

explicit Quantity(double Value, const Unit& unit=Unit());
Then the compiler will tell us where it's wrong.


Btw,
In the FreeCAD this can be triggered e.g. by this:

Create a Pad
Switch Type to anything
Switch Type back to Length
This is already fixed in git commit 869e78
Fat-Zer
Posts: 171
Joined: Thu Oct 30, 2014 10:38 pm

Re: [bug] There is a bug about quontities

Postby Fat-Zer » Mon Feb 29, 2016 1:53 pm

wmayer wrote:To find all cases where quantities with doubles is compared can be easily detected by using the keyword "explicit" in front of the constructor of Quantity
Tried it... lots of hits in QuontityParser, but I haven't dig them any further — there were tones of them.
wmayer wrote:This is already fixed in git commit 869e78
oh... have master build a week old... so embracing... so no necessity for a bug I suppose..

PS: There is a close but different bug in the Pocket dialog:
oldLength used to hack around bad format in some ancient files should be of Quantity type too. Otherwise if you enter e.g."5m" then mess with type it turns to 5mm... And after fixing that it will get the same issue as pad.
wmayer
Site Admin
Posts: 17247
Joined: Thu Feb 19, 2009 10:32 am

Re: [bug] There is a bug about quontities

Postby wmayer » Mon Feb 29, 2016 2:21 pm

Tried it... lots of hits in QuontityParser, but I haven't dig them any further yet...
Yes, the yacc files must be adjusted somewhat and re-generated.

For now I commented out the arts that do not compile and I guess I found a bug in the expression engine:
See method OperatorExpression::eval()

Code: Select all

...
    case LTE:
        if (v1->getUnit() != v2->getUnit())
            throw ExpressionError("Incompatible units for + operator");
        output = new NumberExpression(owner, Quantity(v1->getValue() - v2->getValue() < 1e-7));
        break;
    case GTE:
        if (v1->getUnit() != v2->getUnit())
            throw ExpressionError("Incompatible units for + operator");
        output = new NumberExpression(owner, Quantity(v2->getValue() - v1->getValue()) < 1e-7); <<=== the parenthesis seems wrong
        break;
...
Btw, the expression error text must be adjusted to the actually used operator.

Then in InputField::bind I found this line:

Code: Select all

    if (prop)
        actQuantity = prop->getValue();
is this the intended behaviour that the quantity loses its unit or should this rather be?

Code: Select all

    if (prop) {
        actQuantity = Base::Quantity(prop->getValue(), prop->getUnit());
    }
Fat-Zer
Posts: 171
Joined: Thu Oct 30, 2014 10:38 pm

Re: [bug] There is a bug about quontities

Postby Fat-Zer » Mon Feb 29, 2016 3:03 pm

wmayer wrote:<<=== the parenthesis seems wrong
Seems both variants doesn't make scenes...
wmayer wrote:Btw, the expression error text must be adjusted to the actually used operator.
I'm not sure if exactly this line is intentionally or not, but there seems to be bug around there... e.g. if the input forbids angle more than 360°, the code will likely allow to input 359 rad...
wmayer
Site Admin
Posts: 17247
Joined: Thu Feb 19, 2009 10:32 am

Re: [bug] There is a bug about quontities

Postby wmayer » Mon Feb 29, 2016 3:38 pm

Fat-Zer wrote:Seems both variants doesn't make scenes...
I could be wrong but I guess since the result is boolean it gets converted to a dimensionless quantity where False becomes 0.0 and True 1.0. But it might be better to have a class BooleanExpression or so to make this clearer.
wmayer
Site Admin
Posts: 17247
Joined: Thu Feb 19, 2009 10:32 am

Re: [bug] There is a bug about quontities

Postby wmayer » Mon Feb 29, 2016 6:27 pm

I opened a feature request issue #0002460. This is then for after the release of 0.16.
eivindkvedalen
Posts: 602
Joined: Tue Jan 29, 2013 10:35 pm

Re: [bug] There is a bug about quontities

Postby eivindkvedalen » Tue Mar 01, 2016 11:30 am

wmayer wrote: Yes, the yacc files must be adjusted somewhat and re-generated.

For now I commented out the arts that do not compile and I guess I found a bug in the expression engine:
See method OperatorExpression::eval()

Code: Select all

...
    case LTE:
        if (v1->getUnit() != v2->getUnit())
            throw ExpressionError("Incompatible units for + operator");
        output = new NumberExpression(owner, Quantity(v1->getValue() - v2->getValue() < 1e-7));
        break;
    case GTE:
        if (v1->getUnit() != v2->getUnit())
            throw ExpressionError("Incompatible units for + operator");
        output = new NumberExpression(owner, Quantity(v2->getValue() - v1->getValue()) < 1e-7); <<=== the parenthesis seems wrong
        break;
...
Btw, the expression error text must be adjusted to the actually used operator.
Yes, this is a bug. I should also probably use the confusion() (or whatever the name was) function also, instead of 1e-7. I'll provide a patch.
wmayer wrote: Then in InputField::bind I found this line:

Code: Select all

    if (prop)
        actQuantity = prop->getValue();
is this the intended behaviour that the quantity loses its unit or should this rather be?

Code: Select all

    if (prop) {
        actQuantity = Base::Quantity(prop->getValue(), prop->getUnit());
    }
This looks like a bug, but I'm not sure. There's also an actUnit in the class, and I haven't studied how these actually interact. To me it seems like the current implementation works, but that may be because we only expose this bug if InputField::getQuantity() is called, and that happens only two places, one where getValue() is called afterwards (effectively removing the unit), and then in double TaskFemConstraintPressure::getPressure(void) const. Also in the second case the unit is removed.

Eivind
wmayer
Site Admin
Posts: 17247
Joined: Thu Feb 19, 2009 10:32 am

Re: [bug] There is a bug about quontities

Postby wmayer » Tue Mar 01, 2016 2:55 pm

Yes, this is a bug. I should also probably use the confusion() (or whatever the name was) function also, instead of 1e-7. I'll provide a patch.
Its name is Precision::Confusion() but you can't use this in the core system since it's part of occ and this we don't want there.
Fat-Zer
Posts: 171
Joined: Thu Oct 30, 2014 10:38 pm

Re: [bug] There is a bug about quontities

Postby Fat-Zer » Wed Mar 02, 2016 3:12 am

Fat-Zer wrote:e.g. if the input forbids angle more than 360°, the code will likely allow to input 359 rad...
I was wrong about that...
wmayer wrote:Its name is Precision::Confusion() but you can't use this in the core system since it's part of occ and this we don't want there.
It would be nice to have such constant for FreeCAD internal use as well ...