pgkirsch commented 8 years ago

Combines troposphere and sutherland models into one

bqpd commented 8 years ago

:100:

whoburg commented 8 years ago

@pgkirsch Is there a precedence among #42, #44, and #45? Should one or more of them be closed? Which ones should be reviewed?

pgkirsch commented 8 years ago

45 supersedes #42 and #44. The commits of the latter two are subsets of #45. In #42 you suggested creating separate PR for some additional changes, but now I'm not sure if this is what you meant.

If you could review #45 that would be great! And I can delete #42 and #44 assuming the newest commits in #45 aren't complete garbage.

whoburg commented 8 years ago

Did an initial review. A few comments:

The atmospheric models are really interesting examples because we really don't know the pressures in advance. I'm still grappling with how we should best deal with the fact that some aircraft design models will 'want' high rho and low mu (e.g. a long endurance aircraft), whereas others want low rho and high speed of sound but maybe also low mu (e.g. a jet transport). Wonder whether separating the models for the various quantities more would help with all the possible cases. No need to act on this yet, just something for us to think about.
I think these models should come with plots of the various quantities changing with height. This might be a nice application for @bqpd's new markdown thing.
Let's try to get these working with refactor -- I'm currently getting a ValueError: infeasible constraint: constant term too large. from Tropopause.

bqpd commented 8 years ago

(yeah markdown thing! and that constant term too large error, if it's coming during as_posyslt1, is a result of now simplifying after substitution...)

whoburg commented 8 years ago

Assuming @bqpd is right about the simplifying after substitution, it would be interesting to go back and see what was returned previously for the same model with no substitution. If it was primal Infeasible, that would make sense. But if it was unknown, then this would be an example of substitution licking one class of unkown errors.

pgkirsch commented 8 years ago

It's not clear to me whether the constant term too large is a GPkit bug, or something that needs to be changed in the model.

pgkirsch commented 8 years ago

Problem fixed by removing the constraint T == 216.65*units.K and adding substitutions = {'T': 216.65}.

bqpd commented 8 years ago

Huh... That's not great. This seems like a bug. On Mar 22, 2016 10:03, "pgkirsch" notifications@github.com wrote:

Problem fixed by removing the constraint T == 216.65*units.K and adding substitutions = {'T': 216.65}.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/hoburg/gpkit-models/pull/45#issuecomment-199828964

pgkirsch commented 8 years ago

Actually, I think it was me being dumb, because T already had a value assigned to it, so the substitutions = {'T': 216.65} isn't necessary either.

bqpd commented 8 years ago

Okay, phew. Not sure why the equality constraint was causing a new error if the model was solving before...it should only have raised that error in a model that previously just didn't solve. @pgkirsch was this an example of a valid program broken by the refactor?

pgkirsch commented 8 years ago

No I don't think so. As far as I can tell the refactor just wasn't happy because there was a constraint saying 216.65 == 216.65, and I don't really blame it (the now-personified refactor).

pgkirsch commented 8 years ago

@whoburg RE: our conversation earlier about syntax to implement your ConstraintSet idea, I'm not sure if what you suggested is possible.

x = Variable('x')
cs = ConstraintSet([x >= 1])
cs['x']

returns an error and I couldn't find examples in the unit tests of anything similar. Is there another solution or should this become a feature request?

bqpd commented 8 years ago

Oops, that functionality is in CostedConstraintSet but not in ConstraintSet (for no good reason).

pgkirsch commented 8 years ago

I've tried implementing it in ConstraintSet (by copying the __getitem__ method of CCS), but I've run into an absolute rabbit hole of circular import errors.... Any chance we can clean up some of these __init__.py's?

bqpd commented 8 years ago

Oh that's right. Variable requires Monomial requires ConstraintSet...

Sure, there's a chance! I don't see any good way to remove all of them with the current Nomial structure. For the meantime what about a 'user-facing' ConstraintSet which has these UI bells and whistles and doesn't require a cost?

whoburg commented 8 years ago

We're getting some inheritance bloat at the top level with Model, ConstraintSet, CostedConstraintSet, and SignomialProgram. I think we need to clarify which of these are user facing and worth inheriting from for user models.

As I suggested in https://github.com/hoburg/gpkit/issues/599, perhaps CostedConstraintSet could be user facing and renamed to something like SGPConstraintSet (meaning that it's a set of constraints that plays nicely with sequential GP solution techniques)?

bqpd commented 8 years ago

But in this case we don't have a cost, we just want a ConstraintSet.

Where's the inheritance from CostedConstraintSet and SignomialProgram occurring? Really I think users/models should only inherit from ConstraintSet (we can rename the current internal-use constraint set class) and Model.

whoburg commented 8 years ago

ok that works too. It seems to me that often the choice of objective (cost) occurs during model use, not during model definition / implementation.

What's the current guideline on whether to inherit from Model vs ConstraintSet?

bqpd commented 8 years ago

After the new ConstraintSet, it'll be whether the model in question should be able to be solved on its own.

whoburg commented 8 years ago

Model and the new user facing ConsraintSet seem conceptually very similar. I hate to advocate for multiple ways to __init__, but would it make sense to combine them and support both __init__(self, constraints, **kwargs) and __init__(self, cost, constraints, **kwargs)?

bqpd commented 8 years ago

You mean __init__(self, *args, **kwargs)? :p

bqpd commented 8 years ago

We could require the user to specify self.cost during the init if they wanted a cost.

whoburg commented 8 years ago

yes yes, of course that can be the signature -- but it requires implementation to make it work :)

I'm advocating for:

Maintaining backwards compatibility
Giving users a single suggested class to inherit their models from -- either Model, or ConstraintSet, or some other name -- with optional cost.

bqpd commented 8 years ago

3) Having a sensical init signature/inheritance structure for documentation/pylint/code clarity.

I fee like we just had this discussion. :p In person again sometime, and meanwhile on another thread?

whoburg commented 8 years ago

yes sorry for the off-topic discussion. @pgkirsch, is this ready for merging?

pgkirsch commented 8 years ago

It currently still inherits from Model. Is that ok with you?

whoburg commented 8 years ago

yes, if we're going to change that it might be easier to do so in one fell swoop for all gpkit-models.

@bqpd, do we need a new issue for being able to inherit from ConstraintSet?

bqpd commented 8 years ago

Sounds good! I'll make it right now.

pgkirsch commented 8 years ago

@whoburg Requested changes made and ready to be merged

pgkirsch commented 8 years ago

errr.... continuous improvement?

convexengineering / gplibrary

Atmosphere #45

45 supersedes #42 and #44. The commits of the latter two are subsets of #45. In #42 you suggested creating separate PR for some additional changes, but now I'm not sure if this is what you meant.