v0.2

[pgkirsch]

This pull request makes GPfit object oriented, with fit-type-specific sub-classes (e.g. SoftmaxAffine) derived from a _Fit parent class #100.

These classes have print and plot methods (plot_surface and plot_slices for 2D fits). As part of this change this PR also includes the ability to save fits both to pickle and to a text file (related to #44).

The documentation also gets a facelift, a restructure, installation instructions (#69) and a tutorial page (#7)

Also closes #8, #33, #54, #104.

[pgkirsch]

@bqpd @whoburg I think this is 80% done, but I would like your feedback/advice on a couple things:

1. Should this be v1.0 or v0.2? It completes changes the primary API so by semantic versioning rules it should be a major release, but it also feels silly to release v1.0 when GPkit hasn't even technically released v1.0 yet.
2. As currently written, a user creates instances of the Fit subclasses, e.g. f = ImplicitSoftmaxAffine(...) but that's kind of a cumbersome UX. I was thinking the interface should be f = Fit() where the default type is ISMA. But SoftmaxAffine etc. all subclass Fit as the base class. Any thoughts on how to resolve this? My initial thought was to rename the current base class Fit to BaseFit (or similar) and then use that as the base class for MaxAffine, SoftmaxAffine, ImplicitSoftmaxAffine. Fit would then be the user-facing class.
3. Related: any thoughts on how to slice and dice fit.py -- it's currently pretty long and contains both the base class and the 3 sub-classes.

[bqpd]

1. naw go for 1.0, that seems right!
2. having users create named fits seems fine -- they'll probably assign it to a variable called "fit" anyway
3. how about moving ImplicitSoftmaxAffine etc. to a different file?

[pgkirsch]

1. Cool thanks!
2. It's just a lot to type... And in a practical context, a user generally just wants the best quality fit they can get. Is there a downside to using a Fit alias?
3. I think that's the play. But then it's like should each class have it's own module etc. Anyway probably no need to obsess over it too much, just wanted to see if you had any strong feelings.

[pgkirsch]

After doing a little more research, it looks like the best-practice way of doing this would be an Object Factory.

[pgkirsch]

Ha! I just realized that one way to resolve this is to create a function (fit) that returns the appropriate object. So this overhaul may be backwards compatible after all...

[whoburg]

@pgkirsch great work on the overhaul.

1. I'm good with v 1.0 as well.
2. One option is roughly matching scikit-learn (see e.g. https://scikit-learn.org/stable/modules/linear_model.html#lasso), where there are many types of linear_model, e.g. linear_model.Lasso, linear_model.RidgeCV, linear_model.ElasticNet. Yes these are long to type but I tend to like giving users full control. That said, scikit-learn is certainly not the only package to pattern after, and if you see good reasons to go another way I'm all for it.
3. No strong feelings on single module vs splitting up as long as we're not breaking the pylint module line count limit (1000 lines?)

[pgkirsch]

Thanks @whoburg.

1. I'm torn on this. Now that the fit interface still exists (and is backwards compatible) it feels appropriate to v0.2. The only non-backwards compatible interface changes are the eliminated of plot_fit_1d and print_fit but I think I was probably the only person who used those.
2. What I think I've settled on is a best of both worlds that allows users to directly specify the class of fit they want to use (MaxAffine/SoftmaxAffine/ImplicitSoftmaxAffine) or just use the fit convenience function for the new and/or lazy user (see fit.py for this very short function). The new docs explains that both are options:
3. Ok. All are well below that length fortunately.

[whoburg]

1. I'm also just fine with v0.2. No strong feelings from me on this.
2. That looks awesome. Love it.

[pgkirsch]

Think I'm all done on this now! @bqpd @whoburg ready for review! (as much as you have the time or inclination to provide)

[pgkirsch]

Thanks for the review @whoburg! Happy to finally be able to clean GPfit up a little. It looks like tests are only failing on Reynolds (as on master, not sure why) so I will see if Marshall or Berk are able to help us with this.

[bqpd]

test this please

[bqpd]

the problem is reynolds is using python3.5...

[bqpd]

@galbramc it looks like the python we get on reynolds with virtualenv --python=python3 $WORKSPACE/venv2_gpfit is python 3.5, and after sshing in it looks like that's the only one installed. Since that's about to be deprecated (and doesn't support @pgkirsch's fancy format strings), might this be the time to install a newer version?

[galbramc]

I can't easily install a new version of python. However, reynolds is running Ubuntu 16.04 and needs to be upgraded to 20.04. I was going to start working on that upgrade some time next week. That will give you python 3.8.

[bqpd]

fabulous!

On Thu, Jul 29, 2021, 16:47 Marshall Galbraith @.***> wrote:

I can't easily install a new version of python. However, reynolds is running Ubuntu 16.04 and needs to be upgraded to 20.04. I was going to start working on that upgrade some time next week. That will give you python 3.8.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/convexengineering/gpfit/pull/105#issuecomment-889444342, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALKAGB54WOO55JPVBDYCITT2G44VANCNFSM5BC33E2A .

[pgkirsch]

Thanks @galbramc!

[pgkirsch]

@bqpd hopefully https://github.com/convexengineering/gpfit/pull/105/commits/7b41fb1da934ac4b0065227001f51da2d3b2b1e7 addresses your comment RE: unpacking log transform

[bqpd]

yup that looks good!

[pgkirsch]

@galbramc any chance you have time to upgrade Reynolds this week? I would also be totally fine with only using Macy's for GPfit tests for the time being, if that would be an easier change to make. Happy to make this change myself if I have permission and someone can point me in the right direction to get started.

[galbramc]

I'm waiting for another computer we ordered to arrive before I head to campus to upgrade reynolds. I was hoping it would arrive last week, but it has still not shown up. So now I'm less certain when I'll be able to upgrade reynolds...

[galbramc]

The unit tests are failing on macys as well though, so something is not right with this PR.

[pgkirsch]

Ah ok thanks for the update @galbramc. And thanks for pointing that out -- I had stopped checking the reason for failure so missed that it was failing on Macys too. More impetus to fix #101.

[galbramc]

test this please

[galbramc]

reynolds has been upgraded. The testing errors are now legitimate errors.

[pgkirsch]

Thank you @galbramc! 😭

[galbramc]

test this please

[pgkirsch]

Huh pylint tests passing again, hurray. Hadn't made the pylintrc change yet.

[galbramc]

test this please

[galbramc]

test this please

[galbramc]

The pylint success was a false positive. Jenkins was still not configured quite right, and there was a bug in the Jenkins script for running pyling. Note however that some pylint warnings are actually the master branch of gpfit.

[pgkirsch]

Huh interesting that those warnings don't show up when I run pylint locally. Is Jenkins using a different (later?) version of pylint? (I'm apparently using 2.6.0). Where do I specify which version of pylint I want Jenkins to use given that it's not a package requirement?

[galbramc]

The scripts create a virtualenv and installs the latest version of pylint. All the script does is runs the pylint.sh script in the gpfit repo otherwise.

[pgkirsch]

Ah that was it -- updating to latest pylint (2.10) caught those 5 errors. Now fixed, hopefully the pylint tests pass on Jenkins now..

[galbramc]

You got pylint fixed. Where are you expecting the artifacts directory to be located?

[pgkirsch]

Sorry for the slow follow-up @galbramc. I am expecting it to be in gpfit/tests/artifacts/.

[galbramc]

So do you expect gpfit/tests to be the working directory when you run the tests?

[galbramc]

test this please

[galbramc]

This test keeps randomly failing:

======================================================================
ERROR [9.904s]: test_ex2_mosek_cli (gpfit.tests.t_examples.TestExamples)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/jenkins/workspace/CE_gpfit_PR_unit_tests/buildnode/reynolds/gpkit/gpkit/tests/helpers.py", line 55, in test
    testfn(name, import_dict, path)(self)
  File "/jenkins/workspace/CE_gpfit_PR_unit_tests/buildnode/reynolds/gpkit/gpkit/tests/helpers.py", line 91, in test
    getattr(self, name)(imported[name])
  File "/home/jenkins/workspace/CE_gpfit_PR_unit_tests/buildnode/reynolds/gpfit/tests/t_examples.py", line 44, in test_ex2
    self.assertTrue(example.fsma.errors["rms_rel"] < 1e-3)
AssertionError: False is not true

[galbramc]

Ok, we got green on reynolds, failing tests on macys, and I don't know what this is on Windoze:

Traceback (most recent call last):
  File "C:\Users\jenkins\workspace\CE_gpfit_PR_unit_tests\buildnode\windows10x64\docs\source\examples\ex1.py", line 3, in <module>
    from gpfit.fit import MaxAffine, SoftmaxAffine, ImplicitSoftmaxAffine
  File "c:\users\jenkins\workspace\ce_gpfit_pr_unit_tests\buildnode\windows10x64\gpfit\__init__.py", line 4, in <module>
    from .fit import fit
  File "c:\users\jenkins\workspace\ce_gpfit_pr_unit_tests\buildnode\windows10x64\gpfit\fit.py", line 2, in <module>
    from .classes import MaxAffine, SoftmaxAffine, ImplicitSoftmaxAffine
  File "c:\users\jenkins\workspace\ce_gpfit_pr_unit_tests\buildnode\windows10x64\gpfit\classes.py", line 10, in <module>
    from .constraint_set import FitConstraintSet
  File "c:\users\jenkins\workspace\ce_gpfit_pr_unit_tests\buildnode\windows10x64\gpfit\constraint_set.py", line 5, in <module>
    from gpkit.small_scripts import initsolwarning, appendsolwarning

ImportError: cannot import name 'initsolwarning' from 'gpkit.small_scripts' (C:\Users\jenkins\workspace\CE_gpfit_PR_unit_tests\buildnode\windows10x64\venv2_gpfit\lib\site-packages\gpkit\small_scripts.py)

[pgkirsch]

So do you expect gpfit/tests to be the working directory when you run the tests?

Yes

This test [test_ex2_mosek_cli] keeps randomly failing:

Weird, even when I free up the random seed locally I don't get anything close to 1e-3 but maybe I should relax that for now.

Ok, we got green on reynolds

Whoop thank you!

failing tests on macys

Hm I thought random seeds were platform independent.. Will look into these.

ImportError: cannot import name 'initsolwarning' from 'gpkit.small_scripts'

Ah, unfortunately GPfit requires bleeding edge GPkit (more recent than the latest release on PyPI). Does the windows VM build from latest GPkit release whereas the others built from latest GPkit master on GitHub? Is it possible to do the same on the windows machine? If not @bqpd may be able to make a 0.9.9.9 release of GPkit that should fix this.

[galbramc]

Could you change the assert to an assertLess so we can see what the result actually is in the console?

I've made some updates to the scripts, but I had to power down Jenkins due to a scheduled power outage tomorrow. I'm heading out on some vacation, and will being it back up tomorrow evening when I'm back.

[pgkirsch]

@galbramc sorry for the slow response. That week was hectic and I was on vacation this past week (nevertheless I should have let you know). Changed to assertLess as recommended.

[galbramc]

test this please

[galbramc]

ok we now get random failures on reynolds. take a look at the last two builds

[galbramc]

test this please

[pgkirsch]

Hm. It's nice to see the others passing at least! It seems like there must be some fundamental (maybe embarrassingly obvious) thing I don’t understand about how different OS’s do computation that would explain why these tests pass on Macys and Windows but not Reynolds. Does anything come to mind @galbramc?

As far as next step, the obvious (albeit lazy) thing to do here would be to relax the tolerance to 1e-2 for the fsma tests and call it good. Otherwise I'm not sure what other options exist.

[galbramc]

I don't know anything about how the fsma tests work or what they are testing, so it's very hard for me to give advice here. I don't know if a tolerance of 1e-2 means you actually have a false positive, or if that is good enough. We need guidance from higher powers here (@whoburg)...

[bqpd]

test this please

[bqpd]

I've ssh'd in to reynolds and am running the build script, but haven't reproduced the error in four tries...

@galbramc are there any executables/environment variables that would be different in my user account compared to jenkins'? I haven't modified my bashrc at all and it's all but identical to jenkins'

EDIT: wait I swear I can read trying again with those new PATHs in that bashrc. deleted comment above bc it might have sensitive info

[bqpd]

even with source /home/jenkins/.bashrc not getting any failures running source $WORKSPACE/JenkinsGPkit/gpfit_unit_tests.sh (with WORKSPACE=/home/eburn/gpfit), which is what the PR unit tests do