Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Documentation bug fixes and Sphinx refactor (replace stray Python/.txt with ..literalinclude) #1508

Closed
wants to merge 11 commits into from

Conversation

mtwichan
Copy link

@mtwichan mtwichan commented Jul 19, 2020

Issue: #1280 : make sure all code in docs is in a running example (#303)

Status:

  • Currently, I am naively running all python files in the examples folders and tackling files containing errors.

CC: @bqpd

@acdl-jenkins
Copy link

Can one of the admins verify this patch?

@bqpd
Copy link
Contributor

bqpd commented Jul 19, 2020

add to whitelist

@bqpd
Copy link
Contributor

bqpd commented Jul 20, 2020

this is fabulous! looks like the linting of examples is throwing a fit, while the unit tests are failing because not all files in the examples directory now run individually. Will do a full code review soon!

@@ -5,22 +5,9 @@ Checking for result changes
===========================
Tracking the effects of changes to complex models can get out of hand;
we recommend saving solutions with ``sol.save()``, then checking that new solutions are almost equivalent
with ``sol1.almost_equal(sol2)`` and/or ``print sol1.diff(sol2)``, as shown below.
with ``sol1.almost_equal(sol2)`` and/or ``print(sol1.diff(sol2))``, as shown below.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops good catch

Copy link
Contributor

@bqpd bqpd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

didn't get through all of them, but here's partial notes

@@ -0,0 +1,12 @@
import pickle
... # build the model
sol = m.solve()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, should probably create a dummy model here

Copy link
Author

@mtwichan mtwichan Jul 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a dummy model here: 33588b9

I noticed when running last_verified_sol = pickle.load(open("last_verified.sol")) here: https://github.com/convexengineering/gpkit/pull/1508/files#diff-79562c84e117012e71221b6d88dd595bR23

I was getting this error:

Traceback (most recent call last):
  File "checking_result_changes.py", line 23, in <module>
    last_verified_sol = pickle.load(open("last_verified.sol"))
  File "/usr/lib/python3.6/codecs.py", line 321, in decode
    (result, consumed) = self._buffer_decode(data, self.errors, final)
UnicodeDecodeError: 'utf-8' codec can't decode byte 0xff in position 330: invalid start byte

so I changed the code to: last_verified_sol = pickle.load(open("last_verified.sol", mode="rb")) and it worked successfully.

Not sure if this is a bug or not.

@@ -0,0 +1,6 @@
# create a Monomial term xy^2/z
x = Variable("x")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs to import from gpkit

@@ -0,0 +1,5 @@
# create a Posynomial expression x + xy^2
x = Variable("x")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs to import from gpkit

@@ -0,0 +1,8 @@
# consider a block with dimensions x, y, z less than 1
# constrain surface area less than 1.0 m^2
x = Variable("x", "m")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs to import from gpkit

@@ -0,0 +1 @@
Sankey(M).sorted_by('constraints', 11)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alas this one probably can't be a code snippet and should be left as inline rst code :(

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. For code snippets like this that cannot be run, I will leave them inline.

@@ -0,0 +1 @@
from gpkit import Variable, VectorVariable, Model
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be part of a larger file. this particular line can be excerpted as part of the literalquote syntax

Copy link
Author

@mtwichan mtwichan Jul 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the literalquote syntax? I searched it up in Sphinx but had no luck. Alternatively, how do you feel about keeping trivial lines of code like this one inline?

@@ -0,0 +1,9 @@
from gpkit import Variable, Model
from gpkit.constraints.tight import Loose
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be from gpkit.constraints.loose, lol. This was an unknown bug in the documentation!

@@ -0,0 +1 @@
Sankey(M).diagram(left=60, right=90, width=1050)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as with other Sankey probably can't be run as example code

@@ -0,0 +1 @@
print (sol.table())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be part of a larger file

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made this inline.

@@ -0,0 +1,3 @@
print ("The optimal value is %s." % sol["cost"])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be part of a larger file

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made this inline.

z = VectorVariable(2, "z")
p = x*y*z
assert all(p.sub({x: 1, "y": 2}) == 2*z)
assert all(p.sub({x: 1, y: 2, "z": [1, 2]}) == z.sub(z, [2, 4]))
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is throwing this error:

Traceback (most recent call last):
  File "substituting_multiple_values.py", line 9, in <module>
    sub_two = p.sub({x: 1, y: 2, "z": [1, 2]}).any() == z.sub(z, [2, 4]).any() 
  File "/home/matthew/.local/lib/python3.6/site-packages/gpkit/nomials/array.py", line 182, in sub
    return self.vectorize(lambda nom: nom.sub(subs, require_positive))
  File "/home/matthew/.local/lib/python3.6/site-packages/gpkit/nomials/array.py", line 178, in vectorize
    return vec_recurse(self, function, *args, **kwargs)
  File "/home/matthew/.local/lib/python3.6/site-packages/numpy/lib/function_base.py", line 1972, in __call__
    return self._vectorize_call(func=func, args=vargs)
  File "/home/matthew/.local/lib/python3.6/site-packages/numpy/lib/function_base.py", line 2042, in _vectorize_call
    ufunc, otypes = self._get_ufunc_and_otypes(func=func, args=args)
  File "/home/matthew/.local/lib/python3.6/site-packages/numpy/lib/function_base.py", line 2002, in _get_ufunc_and_otypes
    outputs = func(*inputs)
  File "/home/matthew/.local/lib/python3.6/site-packages/gpkit/nomials/array.py", line 24, in vec_recurse
    return function(element, *args, **kwargs)
  File "/home/matthew/.local/lib/python3.6/site-packages/gpkit/nomials/array.py", line 182, in <lambda>
    return self.vectorize(lambda nom: nom.sub(subs, require_positive))
  File "/home/matthew/.local/lib/python3.6/site-packages/gpkit/nomials/variables.py", line 83, in sub
    return Monomial.sub(self, *args, **kwargs)
  File "/home/matthew/.local/lib/python3.6/site-packages/gpkit/nomials/math.py", line 171, in sub
    return Signomial(self.hmap.sub(substitutions, self.varkeys),
  File "/home/matthew/.local/lib/python3.6/site-packages/gpkit/nomials/map.py", line 104, in sub
    if parsedsubs or not substitutions:
ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()

Haven't quite been able to fix it.

from gpkit.small_scripts import mag

x = Variable("x", "m")
xvk = x.varkeys.values()[0]
Copy link
Author

@mtwichan mtwichan Jul 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 6 throws:

Exception has occurred: TypeError
'dict_values' object does not support indexing
  File "/home/matthew/Desktop/the_gpkit/docs/source/examples/substituting_nonnumeric_values.py", line 6, in <module>
    xvk = x.varkeys.values()[0]

which may be because of a Python 2 artifact, where the original function returned a list, and now in Python3 it returns a view. One solution is to wrap list(x.varkets.values()) see: Link . If I do list(x.varkeys)[0] I get the right value.

Line 16 than throws:

Exception has occurred: TypeError
string indices must be integers
  File "/home/matthew/Desktop/the_gpkit/docs/source/examples/substituting_nonnumeric_values.py", line 16, in <module>
    assert abs(expected - mag(x.sub(x_, y_).c)) < 1e-6

Since, x is gpkit.Variable(x [m]), I think x.sub(x_, y_) is invalid because x contains a single variable. If I run the code without y_ it runs fine, until I pass in a string "x" into x.sub(x_) which throws:

x.sub('x') ValueError: could not convert string to float: 'x'

If I comment out the assert, and add list(x.varkeys)[0] everything runs fine and line 20 throws an error as it should. I think I may need your insight.

from gpkit import Variable
x = Variable("x")
p = x**2
assert p.sub(x, 3) == 9
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Traceback (most recent call last):
  File "substitutions.py", line 5, in <module>
    assert p.sub(x, 3) == 9
  File "/home/matthew/.local/lib/python3.6/site-packages/gpkit/nomials/math.py", line 171, in sub
    return Signomial(self.hmap.sub(substitutions, self.varkeys),
  File "/home/matthew/.local/lib/python3.6/site-packages/gpkit/nomials/map.py", line 107, in sub
    fixed, _, _ = parse_subs(varkeys, substitutions)
  File "/home/matthew/.local/lib/python3.6/site-packages/gpkit/nomials/substitution.py", line 23, in parse_subs
    for var in substitutions:
TypeError: 'Variable' object is not iterable

Copy link
Contributor

@1ozturkbe 1ozturkbe Jul 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An easy fix is:

p.sub({x:3}).value == 9

The subs are always dicts.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops yup it looks like a lot of these examples are from when I had a little "convenience" syntax...that nobody but me used...so I removed it.

x = Variable("x")
p = x**2
assert p.sub(x, 3) == 9
assert p.sub(x.varkeys["x"], 3) == 9
Copy link
Contributor

@1ozturkbe 1ozturkbe Jul 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

p.sub({x.key :3}) I think is intended here.

@1ozturkbe
Copy link
Contributor

This all looks great! Super amped to have the documentation up to date and tested!

@mtwichan
Copy link
Author

Transfer PR to: #1509

@mtwichan mtwichan closed this Jul 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants