Skip to content

Fixedvarkeys#787

Merged
whoburg merged 12 commits intomasterfrom
fixedvarkeys
Aug 6, 2016
Merged

Fixedvarkeys#787
whoburg merged 12 commits intomasterfrom
fixedvarkeys

Conversation

@bqpd
Copy link
Contributor

@bqpd bqpd commented Aug 4, 2016

Fix varkeys on model creation, and make it easy to add to that list. Closes #784 and makes convexengineering/gplibrary#17 possible.

@whoburg
Copy link
Collaborator

whoburg commented Aug 4, 2016

double-check the ticket numbers this closes? 17 must be a typo.

@bqpd
Copy link
Contributor Author

bqpd commented Aug 4, 2016

Oops, yep, meant in gpkit-models

gpkit/keydict.py Outdated
def add(self, *args):
"Adds all args to the keyset"
for arg in args:
self[arg] = None
Copy link
Collaborator

Choose a reason for hiding this comment

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

python's set.add takes exactly one argument. I think we should match that interface here. https://docs.python.org/2/library/sets.html#sets.Set

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then how should we add multiple?

Copy link
Collaborator

Choose a reason for hiding this comment

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

same way people add multiple things to a set -- I assume they write a for loop? Or also implement union, which takes an iterable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

@whoburg
Copy link
Collaborator

whoburg commented Aug 5, 2016

This branch works for d8 but not for another external model. Issue appears to be with how model names are appended to variables.

def reset_varkeys(self, init_dict=None):
"Goes through constraints and collects their varkeys."
varkeys = KeySet()
if not init_dict is None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think preferred syntax is if init_dict is not None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

@bqpd
Copy link
Contributor Author

bqpd commented Aug 5, 2016

@whoburg, see if commenting out the last line in Model._add_modelname_tovars fixes it

@whoburg
Copy link
Collaborator

whoburg commented Aug 5, 2016

nope, commenting out that line did not fix it. I'll debug a bit

@whoburg
Copy link
Collaborator

whoburg commented Aug 5, 2016

model that's not getting model names appended is a LinkedConstraintSet, if that matters

@whoburg
Copy link
Collaborator

whoburg commented Aug 5, 2016

Here's a MWE for not adding modelname to varkeys:

from gpkit import Variable, VectorVariable, Model

class Thing(Model):
    "a thing"
    def __init__(self, n, **kwargs):
        a = VectorVariable(n, "a", "g/m")
        b = VectorVariable(n, "b", "m")
        c = Variable("c", 17/4., "g")
        Model.__init__(self, None, [a >= c/b], **kwargs)


t = Thing(3)
print t.varkeys

output is

{c: None, b_(1,): None, a_(1,): None, a_(0,): None, a_(2,): None, b_(2,): None, b_(0,): None}

@whoburg
Copy link
Collaborator

whoburg commented Aug 5, 2016

added that MWE as a test to fix

@bqpd
Copy link
Contributor Author

bqpd commented Aug 6, 2016

Passes the test locally, now.

@whoburg
Copy link
Collaborator

whoburg commented Aug 6, 2016

ok excellent, that was the issue. I'm going to try rebasing this onto master.

@whoburg
Copy link
Collaborator

whoburg commented Aug 6, 2016

@mayork, would you mind switching to this branch briefly and confirming your model(s) run fine?

@whoburg whoburg mentioned this pull request Aug 6, 2016
@whoburg
Copy link
Collaborator

whoburg commented Aug 6, 2016

ok, the one ubuntu16 test failure was not caused by this; I'm ready to merge this.

@mayork
Copy link
Contributor

mayork commented Aug 6, 2016

@whoburg I'm having trouble checking this out...I'm not sure if I'm doing something stupid but I can checkout branches like master equality and breakdown but get the following error when I try to check this out...it's called fixedvarkeys right?

dhcp-18-111-87-78:gpkit mayork$ git checkout fixedvarkeys
error: pathspec 'fixedvarkeys' did not match any file(s) known to git.
dhcp-18-111-87-78:gpkit mayork$ git checkout equality
Already on 'equality'
Your branch is up-to-date with 'origin/equality'.

@whoburg
Copy link
Collaborator

whoburg commented Aug 6, 2016

do git fetch first, and then you should be able to check out fixedvarkeys.

@mayork
Copy link
Contributor

mayork commented Aug 6, 2016

okay got the branch installing and testing right now

@mayork
Copy link
Contributor

mayork commented Aug 6, 2016

sorry slight delay I think I just broke the atmosphere model

@mayork
Copy link
Contributor

mayork commented Aug 6, 2016

@whoburg it works...solved the model in fewer iterations also.

@whoburg whoburg merged commit 219cb17 into master Aug 6, 2016
@whoburg whoburg deleted the fixedvarkeys branch August 6, 2016 14:45
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.

3 participants