Refactor #61

bqpd · 2016-12-22T06:05:52Z

as part of digging into the gpfit code I cleaned up some logic in fit.py...

@mjburton11, could you check this still works with your stuff?

@whoburg, ready for review.

mjburton11 · 2016-12-22T14:47:09Z

@bqpd, the renaming of u to u_fit caused some problems in evaluate_fit.py that I was able to fix. (pull my latest commit on this branch) However, running gassolar/environment/wind_fit.py in commit convexengineering/gassolar@d65428c gives this error:

---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
/Users/mjburton11/MIT/GPKIT/gpkit-projects/gas_solar_trade/gassolar/environment/wind_fit.py in <module>()
    100                     print "rms iter=%d" % i
    101                 np.random.seed(i)
--> 102                 cns, rm = fit(X, Y, 4, "SMA")
    103                 rms.append(rm)
    104                 if rm > 0.05:

/Users/mjburton11/anaconda/lib/python2.7/site-packages/gpfit/fit.pyc in fit(xdata, ydata, K, ftype)
    105     elif ftype == "SMA":
    106         # constraint of the form w^alpha >= c1*u1^exp1 + c2*u2^exp2 +....
--> 107         lhs, rhs = w**alpha, monos.sum()
    108         print_SMA(A, B, alpha, d, K)
    109     elif ftype == "MA":

/Users/mjburton11/MIT/GPKIT/gpkit/gpkit/nomials/array.pyc in sum(self, *args, **kwargs)
    199             i = it.multi_index
    200             it.iternext()
--> 201             cs.extend(mag(self[i].cs).tolist())
    202             exps.extend(self[i].exps)
    203         if units:

AttributeError: 'float' object has no attribute 'cs'

bqpd · 2016-12-22T17:18:08Z

The bottom part of the error message seems to be missing...

bqpd · 2016-12-28T21:11:44Z

@mjburton11, what's the full error message above?

bqpd · 2016-12-28T21:13:21Z

Oh I got it. Fixing...

mjburton11 · 2016-12-28T21:13:46Z

haha just updated the comment...

bqpd · 2016-12-28T21:29:26Z

@mjburton11, I think the latest commit fixes the bug

mjburton11 · 2016-12-28T21:31:26Z

@bqpd, now getting the following error:

---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
/Users/mjburton11/MIT/GPKIT/gpkit-projects/gas_solar_trade/gassolar/environment/wind_fit.py in <module>()
    144     colnames = np.append(colnames, "alpha")
    145     colnames = np.insert(colnames, 0, "latitude")
--> 146     df.columns = colnames
    147     # df.to_csv("windaltfitdata1.csv")

/Users/mjburton11/anaconda/lib/python2.7/site-packages/pandas/core/generic.pyc in __setattr__(self, name, value)
   2159         try:
   2160             object.__getattribute__(self, name)
-> 2161             return object.__setattr__(self, name, value)
   2162         except AttributeError:
   2163             pass

pandas/src/properties.pyx in pandas.lib.AxisProperty.__set__ (pandas/lib.c:42548)()

/Users/mjburton11/anaconda/lib/python2.7/site-packages/pandas/core/generic.pyc in _set_axis(self, axis, labels)
    411 
    412     def _set_axis(self, axis, labels):
--> 413         self._data.set_axis(axis, labels)
    414         self._clear_item_cache()
    415 

/Users/mjburton11/anaconda/lib/python2.7/site-packages/pandas/core/internals.pyc in set_axis(self, axis, new_labels)
   2217         if new_len != old_len:
   2218             raise ValueError('Length mismatch: Expected axis has %d elements, '
-> 2219                              'new values have %d elements' % (old_len, new_len))
   2220 
   2221         self.axes[axis] = new_labels

ValueError: Length mismatch: Expected axis has 8 elements, new values have 14 elements

bqpd · 2016-12-28T21:31:51Z

Actually I think this is better fixed in gpkit...

mjburton11 · 2016-12-28T21:32:12Z

Wait.. this is a pandas error...

bqpd · 2016-12-28T21:34:52Z

Yeah not sure about that pandas error :p. Worked on mine, got a latex error instead

mjburton11 · 2016-12-28T21:36:00Z

@bqpd, ok so when I generate the fit, K=4 SMA, it prints out 4 terms but the constraint it returns only has 2...

bqpd · 2016-12-28T21:41:09Z

Yeah, the first two terms were zero. :-/

bqpd · 2016-12-28T21:41:16Z

Which is what caused the bug.

mjburton11 · 2016-12-28T21:42:48Z

Hmmm... that's not what I'm seeing.

mjburton11 · 2016-12-28T21:43:46Z

Ok now I'm getting this error:

---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
/Users/mjburton11/MIT/GPKIT/gpkit-projects/gas_solar_trade/gassolar/environment/wind_fit.py in <module>()
    100                     print "rms iter=%d" % i
    101                 np.random.seed(i)
--> 102                 cns, rm = fit(X, Y, 4, "SMA")
    103                 rms.append(rm)
    104                 if rm > 0.05:

/Users/mjburton11/anaconda/lib/python2.7/site-packages/gpfit/fit.pyc in fit(xdata, ydata, K, ftype)
    105     elif ftype == "SMA":
    106         # constraint of the form w^alpha >= c1*u1^exp1 + c2*u2^exp2 +....
--> 107         lhs, rhs = w**alpha, monos.sum()
    108         print_SMA(A, B, alpha, d, K)
    109     elif ftype == "MA":

/Users/mjburton11/MIT/GPKIT/gpkit/gpkit/nomials/array.pyc in sum(self, *args, **kwargs)
    199             i = it.multi_index
    200             it.iternext()
--> 201             cs.extend(mag(self[i].cs).tolist())
    202             exps.extend(self[i].exps)
    203         if units:

AttributeError: 'float' object has no attribute 'cs'

bqpd · 2016-12-28T21:44:13Z

Yup, that's the zeros.

bqpd · 2016-12-28T21:44:47Z

making a gpkit pull (convexengineering/gpkit#1019) to address the summing issue. Not sure why the fit has zeros, though; did it not before, or was that just abstracted away before?

mjburton11 · 2016-12-28T21:45:34Z

So sometimes it did have zeros, but with a different np.random seed I could often get a function without a zero...

bqpd · 2016-12-28T21:50:17Z

Did those functions have a lower rmse, or is a 4th-order fit overkill, or is this some weird local minima thing?

bqpd · 2016-12-28T21:52:53Z

@mjburton11, now implements proper summing on gpkit master, so you shouldn't see that 'float' object has no attribute 'cs' error anymore.

mjburton11 · 2016-12-28T23:39:06Z

Ok I'm not getting that error anymore. But the printed fit is still different from the returned constraint:

In [1]: %run wind_fit.py
Fitting for 54 latitude
rms iter=0
w**0.00298794 = 0.045666 * (u_1)**3.05619 * (u_2)**3.31932
    + 0.00105499 * (u_1)**0.0966488 * (u_2)**27.3915
    + 0.634617 * (u_1)**-0.0265261 * (u_2)**0.0118065
    + 0.362956 * (u_1)**0.0510403 * (u_2)**-0.0161858
RMS Error: 0.013 after iter=0, Altitude 40000

...Pandas error...

In [3]: cns
Out[3]: gpkit.PosynomialInequality(w_fit**0.003 >= 0.363*u_fit_(0,)**0.051*u_fit_(1,)**-0.016 + 0.635*u_fit_(0,)**-0.027*u_fit_(1,)**0.012)

bqpd · 2016-12-28T23:42:49Z

That looks like a refactor problem, yeah! For some reason it's dropping the two smaller terms.

bqpd · 2016-12-29T09:52:50Z

Resolved by the latest commit, I think: was a numerical issue with small values of alpha

bqpd · 2016-12-29T09:59:36Z

@mjburton11, re: evaluate_fit.py, what about having the constraints returned by fit have a method that accepts x data?

bqpd · 2016-12-29T10:20:41Z

Doing so seems to also remove some of the iterations that were occurring in wind_fit.py without any visible change to the output:

bqpd · 2016-12-29T18:30:39Z

Now with more refactor...

bqpd · 2016-12-29T18:31:39Z

@mjburton11, @whoburg, this is ready for review.

bqpd · 2016-12-29T18:35:18Z

@galbramc, the example seems unable to import gpfit? Or at least it has this pylint error: "Unable to import 'gpfit.fit'"

bqpd · 2016-12-29T18:41:31Z

@galbramc, fixed it by pip installing gpfit in jenkins

galbramc · 2016-12-29T21:47:37Z

Hmm, didn't realize I had forgotten to install gpfit

mjburton11 · 2016-12-30T21:39:10Z

I've run this through all my stuff and it works fine. I like that the evaluate function can be called from constraint. Much more elegant way to do that...

bqpd · 2016-12-30T22:20:18Z

:) merge away! (this'll just merge into your PR)

…

On Dec 30, 2016 13:39, "Michael Burton" ***@***.***> wrote: I've run this through all my stuff and it works fine. I like that the evaluate function can be called from constraint. Much more elegant way to do that... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#61 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABagGCjcgP8n81zSs3UYb4USoNbM2sWlks5rNXn-gaJpZM4LTq2I> .

mjburton11 · 2016-12-30T22:23:26Z

Should I squash/merge?

bqpd · 2016-12-30T22:27:04Z

Probably regular merge into yours, and we can decide whether to squash from there?

…

On Dec 30, 2016 14:23, "Michael Burton" ***@***.***> wrote: Should I squash/merge? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#61 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABagGANmqv7eMqf-6B3SNdEpzMTwxHKGks5rNYRegaJpZM4LTq2I> .

bqpd · 2016-12-30T22:42:12Z

Deleted branch.

whoburg

Thanks @bqpd, what a huge bunch of improvements! I finally had a chance to review and left a few comments / suggestions. Should we address those with issues / a new PR?

whoburg · 2017-01-02T12:37:13Z

.pylintrc

@@ -60,7 +60,7 @@ confidence=
 # no Warning level messages displayed, use"--disable=all --enable=classes
 # --disable=W"
 #disable=import-star-module-level,old-octal-literal,oct-method,print-statement,unpacking-in-except,parameter-unpacking,backtick,old-raise-syntax,old-ne-operator,long-suffix,dict-view-method,dict-iter-method,metaclass-assignment,next-method-called,raising-string,indexing-exception,raw_input-builtin,long-builtin,file-builtin,execfile-builtin,coerce-builtin,cmp-builtin,buffer-builtin,basestring-builtin,apply-builtin,filter-builtin-not-iterating,using-cmp-argument,useless-suppression,range-builtin-not-iterating,suppressed-message,no-absolute-import,old-division,cmp-method,reload-builtin,zip-builtin-not-iterating,intern-builtin,unichr-builtin,reduce-builtin,standarderror-builtin,unicode-builtin,xrange-builtin,coerce-method,delslice-method,getslice-method,setslice-method,input-builtin,round-builtin,hex-method,nonzero-method,map-builtin-not-iterating
-disable=locally-disabled
+disable=locally-disabled,superfluous-parens,wrong-import-position, fixme, duplicate-code, maybe-no-member


In gpkit our pylint.sh script runs pylint with --disable=fixme. That seems better than disabling here, so that pylint plugins will show todos.

Also not sure I agree with disabling duplicate-code, especially while we're working towards refactor! But I know it's a pain until we get the counts to zero, so maybe we should address that by opening an issue re: duplicate-code.

The duplicate code is in the tests directory, which could probably be allowed to have duplicate code...not that this is the right way to do that, either.

whoburg · 2017-01-02T12:38:40Z

.pylintrc

-
-# Naming hint for function names
-function-name-hint=[a-z_][a-z0-9_]{2,30}$
+function-rgx=[A-Za-z_][a-z0-9_]{2,30}$


what method needs caps?

I might have done search-and-replace, I was so tired of pylint.

whoburg · 2017-01-02T12:40:47Z

.pylintrc

-
-# Naming hint for class names
-class-name-hint=[A-Z_][a-zA-Z0-9]+$
+class-rgx=[t_]?|[A-Z_][a-zA-Z0-9]+$


wouldn't it be better to just update our test classes and modules to follow the expected style?

Definitely!

whoburg · 2017-01-02T12:45:31Z

gpfit/tests/t_lse_implicit.py

@@ -3,14 +3,16 @@
 import numpy as np
 from numpy import array, arange
 from gpfit.lse_implicit import lse_implicit
-from .seed import SEED
+
+SEED = 33404


just curious, why hard-code this instead of importing?

With the local import you can't run the test files as scripts!

whoburg · 2017-01-02T12:54:35Z

oh, this got merged to smaequality, not master. Shall I review over there? (timeframe may take ~1 week)

bqpd · 2017-01-02T18:51:04Z

I think this mostly replaced smaequality! I'll check what the diff is.

bqpd · 2017-01-02T19:06:38Z

Made a new issue for these pylint issues: #63

bqpd · 2017-01-02T19:11:15Z

@whoburg, these changes were identical to those in smaequality, so I merged that branch to master.

bqpd added 2 commits December 21, 2016 22:00

refactor fit

5a6c214

pylint

8b488d7

bqpd changed the base branch from master to smaequality December 22, 2016 06:06

bqpd and others added 2 commits December 21, 2016 22:10

no local imports in tests

5ef173c

changing naming to match new constraint creation

83c589a

bqpd and others added 3 commits December 22, 2016 12:12

explicitly cast d to int

0820120

Merge branch 'refactor' of github.com:hoburg/gpfit into refactor

aa97d8c

change MA evaulation call to match refactor changes

7c05dcb

don't include 0 monomials

cb36e33

revert skipping zeroes

4058638

fix numerical issues in coefficients

ad7915b

add evaluate function to cstrt

48095db

bqpd added 3 commits December 29, 2016 02:45

remove generic_resid_fun and its dimensionality tests

7f956cf

pylint

0f8f5b4

more pylint

7a48d89

additional pylint

34e7d85

pylint disable that jenkins seemingly needs

220802e

typo

0557195

mjburton11 merged commit b0f2b46 into smaequality Dec 30, 2016

bqpd deleted the refactor branch December 30, 2016 22:42

whoburg reviewed Jan 2, 2017

View reviewed changes

Refactor #61

Refactor #61

Conversation

bqpd commented Dec 22, 2016 • edited Loading

mjburton11 commented Dec 22, 2016 • edited Loading

bqpd commented Dec 22, 2016

bqpd commented Dec 28, 2016

bqpd commented Dec 28, 2016

mjburton11 commented Dec 28, 2016

bqpd commented Dec 28, 2016

mjburton11 commented Dec 28, 2016

bqpd commented Dec 28, 2016

mjburton11 commented Dec 28, 2016

bqpd commented Dec 28, 2016

mjburton11 commented Dec 28, 2016

bqpd commented Dec 28, 2016

bqpd commented Dec 28, 2016

mjburton11 commented Dec 28, 2016

mjburton11 commented Dec 28, 2016

bqpd commented Dec 28, 2016 • edited Loading

bqpd commented Dec 28, 2016 • edited Loading

mjburton11 commented Dec 28, 2016 • edited Loading

bqpd commented Dec 28, 2016 • edited Loading

bqpd commented Dec 28, 2016

mjburton11 commented Dec 28, 2016

bqpd commented Dec 28, 2016 • edited Loading

bqpd commented Dec 29, 2016 • edited Loading

bqpd commented Dec 29, 2016

bqpd commented Dec 29, 2016 • edited Loading

bqpd commented Dec 29, 2016

bqpd commented Dec 29, 2016

bqpd commented Dec 29, 2016

bqpd commented Dec 29, 2016

galbramc commented Dec 29, 2016

mjburton11 commented Dec 30, 2016

bqpd commented Dec 30, 2016 via email

mjburton11 commented Dec 30, 2016

bqpd commented Dec 30, 2016 via email

bqpd commented Dec 30, 2016

whoburg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whoburg commented Jan 2, 2017

bqpd commented Jan 2, 2017

bqpd commented Jan 2, 2017

bqpd commented Jan 2, 2017 • edited Loading

bqpd commented Dec 22, 2016 •

edited

Loading

mjburton11 commented Dec 22, 2016 •

edited

Loading

bqpd commented Dec 28, 2016 •

edited

Loading

bqpd commented Dec 28, 2016 •

edited

Loading

mjburton11 commented Dec 28, 2016 •

edited

Loading

bqpd commented Dec 28, 2016 •

edited

Loading

bqpd commented Dec 28, 2016 •

edited

Loading

bqpd commented Dec 29, 2016 •

edited

Loading

bqpd commented Dec 29, 2016 •

edited

Loading

bqpd commented Jan 2, 2017 •

edited

Loading