convexengineering / gpkit

Geometric programming for engineers
http://gpkit.readthedocs.org
MIT License
206 stars 40 forks source link

Error with unit conversion on tight constraints #1390

Closed mvernacc closed 5 years ago

mvernacc commented 5 years ago

I just updated gpkit and am now having an AttributeError from a model that used to run ok. The error comes from Tight.process_result trying to do a pint unit conversion on a variable that is of type numpy.float64, not a pint type. I think I have found a solution to the error.

Error

AttributeError                            Traceback (most recent call last)
/home/mvernacc/repos/gplibrary/gpkitmodels/SP/aircraft/compressible_aero/wing_demo_drag.py in <module>()
    105 
    106 if __name__ == '__main__':
--> 107     main()

/home/mvernacc/repos/gplibrary/gpkitmodels/SP/aircraft/compressible_aero/wing_demo_drag.py in main()
    100 
    101 def main():
--> 102     drag_plots()
    103     plt.show()
    104 

/home/mvernacc/repos/gplibrary/gpkitmodels/SP/aircraft/compressible_aero/wing_demo_drag.py in drag_plots()
     56                 wing_model.substitutions['sweep_c2'] = sweep
     57                 try:
---> 58                     sol = wing_model.solve()
     59                     C_D[i] = sol['variables']['C_D']
     60                     c_l[i] = sol['variables']['c_l']

/home/mvernacc/repos/gpkit/gpkit/constraints/prog_factories.pyc in solvefn(self, solver, verbosity, skipsweepfailures, **kwargs)
    121         else:
    122             self.program, progsolve = genfunction(self)
--> 123             result = progsolve(solver, verbosity, **kwargs)
    124             solution.append(result)
    125         solution.program = self.program

/home/mvernacc/repos/gpkit/gpkit/constraints/gp.pyc in solve(self, solver, verbosity, warn_on_check, process_result, **kwargs)
    231 
    232         if process_result:
--> 233             self.process_result(self.result)
    234         self.result["soltime"] = soltime
    235         return self.result

/home/mvernacc/repos/gpkit/gpkit/constraints/set.pyc in process_result(self, result)
    308         for constraint in self:
    309             if hasattr(constraint, "process_result"):
--> 310                 constraint.process_result(result)
    311         for v in self.unique_varkeys:
    312             if not v.evalfn or v in result["variables"]:

/home/mvernacc/repos/gpkit/gpkit/constraints/set.pyc in process_result(self, result)
    308         for constraint in self:
    309             if hasattr(constraint, "process_result"):
--> 310                 constraint.process_result(result)
    311         for v in self.unique_varkeys:
    312             if not v.evalfn or v in result["variables"]:

/home/mvernacc/repos/gpkit/gpkit/constraints/tight.pyc in process_result(self, result)
     53                     raise ValueError(msg)
     54                 if hasattr(leftsubbed, "magnitude"):
---> 55                     rightsubbed = rightsubbed.to(leftsubbed.units).magnitude
     56                     leftsubbed = leftsubbed.magnitude
     57                 constraint.tightvalues = (leftsubbed, constraint.oper,

AttributeError: 'numpy.float64' object has no attribute 'to'

Context

This model was previously running without this error when I was using gpkit v0.7. This morning I upgraded to the latest gpkit v0.8 by git pull, pip install -e .. Now I get this error when a particular constrain goes un-tight. In some contexts the model solves without raising this error; I believe this is because the constraints all remain tight

Solution

I changed line 54 of tight.py to:

if hasattr(leftsubbed, "magnitude") and hasattr(rightsubbed, "magnitude"):

and the error does not occur.

Suspected cause

I think that I have a constraint which is comparing a dimensionless pint variable to a float variable. Line 54 only checked that leftsubbed was pint-like before calling pint methods on both leftsubbed and rightsubbed.

1ozturkbe commented 5 years ago

Do you have a MWE? It's going to be hard to understand what is going on without this. Or can you send us the links to the repos and the commits you were using so we can reproduce the error?

1ozturkbe commented 5 years ago

I want to understand why this is happening before making a change, instead of hacking it.

mvernacc commented 5 years ago

Do you have a MWE? It's going to be hard to understand what is going on without this. Or can you send us the links to the repos and the commits you were using so we can reproduce the error?

I am working on making a minimum working example

mvernacc commented 5 years ago

Here is the example: https://gist.github.com/mvernacc/5505ea8278f8cacff9ce0b2d447ccf1c It turns out the issue is specifically with comparing a variable dimensioned in radians to a dimensionless number.

1ozturkbe commented 5 years ago

This really is an edge case, and I'm tempted to say that this is somewhat an issue on the part of pint. It allows you to make the comparison:

units('') <= units('radians')

whereas I think you shouldn't be able to. (Tbh I have always been frustrated with pint's handling of dimless variables, since floats do not have a .magnitude, and writing exceptions is a pain.) Let me see what we can do. Thanks for the MWE!

mvernacc commented 5 years ago

Ok, thanks for looking into it.

Perhaps the underlying problem is that I decided to make some of my angles dim'less because I need to do taylor series on them to approximate trig functions. I had left off the units because I though pint would throw a fit about rad + rad**2 + rad**3 + ....

But it turns out that with pint ureg('rad') == ureg('rad**2') !?!?

So I can dimension all my angles with radians and that should solve the problem...

1ozturkbe commented 5 years ago

The funny thing is that sin(radians) returns a ratio of lengths (dimless). If you think about it, a radian is a ratio of lengths (diameter/circumference of radius). And so, I think it's fine if you just do the Taylor expansion without dimensions as intended.

1ozturkbe commented 5 years ago

(yes, I am suggesting here is that the 'unit' radians is tautological.)

1ozturkbe commented 5 years ago

If you don't mind, I am going to close this issue since this is definitely a pint problem. I do suggest bringing it up with pint, I'm sure they would appreciate the MWEs and help with fixes.

1ozturkbe commented 5 years ago

btw, it seems like they have had to hear about this before... and didn't fix it

1ozturkbe commented 5 years ago

here they even talk about Taylor expansions of trig functions...