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

Allowing substituted, unitless signomials in exponents. #1438

Open
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

1ozturkbe
Copy link
Contributor

@1ozturkbe 1ozturkbe commented Sep 28, 2019

This will take a bunch of work, but I wanted this to be a clear WIP subject to discussion.
Things to do (list will grow):

  • Check exp units at constraint creation stage.
  • Check that we don't do float**signomial.
  • Check exp substitution at solution stage.
  • Check substitution into individual monomials.
  • Make sure variables in exps are added to Nomial.varkeys.
  • Make sure variables in exps are added to model.varkeys, but not constrained_varkeys (?).
  • Check printing for Nomial.str_without() for signomial exps.
  • Check differentiation with signomial exps.
  • Make sure substitutions occur as expected after constraint creation.
  • Make sure varkeyvalues are stored.
  • Check how different kinds of substitutions play with signomial exps (sweeps etc.).
  • Check variables substitute into exps are always bounded.
  • Check sens_from_dual.

@1ozturkbe
Copy link
Contributor Author

Already encountering issues in Nomials.str_without, since it expects floats as exponents. Will implement fix.

@1ozturkbe
Copy link
Contributor Author

Alright, as I dig I realize there are more and more things to do. I will be tracking signomials with variable exponents with a self.varexps attribute, and making sure we track these variables shortly.

@whoburg
Copy link
Collaborator

whoburg commented Sep 29, 2019

Big picture strategy discussion. These types of expressions are not GP compatible, but are compatible with the exponential cone (possibly requiring some substitutions to get them into an exponential cone compatible form). So this will play nicely with mosek 9. Do we all agree with that overall picture?

Do we want something along the lines of SignomialsEnabled to switch between GP mode and exponential cone mode? I actually think that might be too overbearing (with mosek 9 support, we're solving using exponential cone instead of GP anyway, so maybe we should let users create this type of expression by default). With that said, I still think we should also provide some transparency to the user about when their program is a pure GP vs an exponential cone program. It's one of the virtues of GPkit that we expose users to a bit of the mathematics underlying their modeling decisions.

Not trying to side track! Just thought it would be a useful high-level discussion.

@1ozturkbe
Copy link
Contributor Author

The primary intended use for this is to allow for obtaining the sensitivities to the exponents. I'm not even thinking about exponential cones atm. But good to think about for the future. I do think that we should strive for mosek 9 compatibility before we consider allowing exponents to be variables anyways.

bqpd
bqpd previously approved these changes Sep 30, 2019
csmap = None # used for monomial-mapping postsubstitution; see .mmap()
expmap = None # used for monomial-mapping postsubstitution; see .mmap()
csmap = None # used for monomial-mapping postsubstitution; see .mmap()
varexps = False # used for signomial exponent operations
Copy link
Contributor

Choose a reason for hiding this comment

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

convention for comments in GPkit is double space between code and #, no alignment

Copy link
Contributor

Choose a reason for hiding this comment

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

I think pylint may complain about one of those or the other?

@bqpd bqpd dismissed their stale review September 30, 2019 14:46

(whoops, thought this was a PR into varexp, not master)

return self.__class__({key: val**other
for (key, val) in self.items()})
return self.__class__({key: val**other
for (key, val) in self.items()})
return NotImplemented
Copy link
Contributor

Choose a reason for hiding this comment

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

this fallback return statement is currently unnecessary

Copy link
Contributor

Choose a reason for hiding this comment

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

lol pylint will catch this though, so I'll stop flagging things I know pylint will catch

@1ozturkbe
Copy link
Contributor Author

Wow, this has been an absolute battle against existing GPkit norms. I can hardly get anything to work, since it involves really messing with NomialData and Nomial. I have figured out that I will likely need to store the signomial exponents using an hmap embedded in an hmap.

@whoburg
Copy link
Collaborator

whoburg commented Oct 2, 2019

The primary intended use for this is to allow for obtaining the sensitivities to the exponents. I'm not even thinking about exponential cones atm. But good to think about for the future. I do think that we should strive for mosek 9 compatibility before we consider allowing exponents to be variables anyways.

Per the issue title, the exponents would be unitless signomials, but it sounds like the intent is for them just to be constant 'variables' such that sensitivities can be extracted. I think this should be made more clear -- is this PR for the general signomial case, or just for fixed variables?

@whoburg
Copy link
Collaborator

whoburg commented Oct 2, 2019

Wow, this has been an absolute battle against existing GPkit norms. I can hardly get anything to work, since it involves really messing with NomialData and Nomial. I have figured out that I will likely need to store the signomial exponents using an hmap embedded in an hmap.

I wonder if we need to think about how NomialData and Nomial get extended beyond GP to the more general exponential cone.

@1ozturkbe
Copy link
Contributor Author

More difficulties... It turns out that differentiating a variable**signomial is non-trivial. Will try and make it neat.

@1ozturkbe
Copy link
Contributor Author

@bqpd I wanted to ask you a question about differentiation in GPkit. Does it exist anywhere in the core code other than tests? (The derivative of var**sig w.r.t a variable in the signomial has a log, and was wondering how much havoc this is going to cause.) I have already scoured the code but couldn't find anything. Let me know.

@bqpd
Copy link
Contributor

bqpd commented Oct 4, 2019 via email

@1ozturkbe
Copy link
Contributor Author

Oh poop of course. Alright, I will try to make a non-intrusive method.

@bqpd
Copy link
Contributor

bqpd commented Oct 17, 2019

todo for Ned:

  • check whether some old speedups (vks / varkeys for example) can be simplified

@1ozturkbe
Copy link
Contributor Author

@bqpd I see that this only breaks test_boundschecking, but what does that f'n actually do? It just seems to have a pass?

@bqpd
Copy link
Contributor

bqpd commented Nov 14, 2019

@1ozturkbe ah, that means it just checks that boundschecking.py in the examples directory runs without raising errors, but doesn't perform any additional tests

@bqpd
Copy link
Contributor

bqpd commented Nov 14, 2019

It looks like the error might just be in the type of error raised by cvxopt, which the test expects to be a ValueError but which is now a RuntimeWarning. I'm a little concerned that the error type has changed; this indicates that the bounds-checking in gp.py is no longer raising the ValueError that it should for an unbounded model like this one, but is instead attempting to solve it. Did you disable boundschecking errors?

@1ozturkbe
Copy link
Contributor Author

umm not that I know of! it is possible I created a bug since I have been messing with bounds (since signomial exponents, unsubstituted, cannot bound anything).

@bqpd
Copy link
Contributor

bqpd commented Mar 7, 2020

@1ozturkbe I'm afraid all the recent changes on master have created some conflicts here...but on the plus side, the removal of 1700 lines of code from GPkit will probably make it easier to implement :p

@1ozturkbe
Copy link
Contributor Author

It's alright. This was quite the mongo chunk of code to do at once... at least I get less code to work with, you are right!

@1ozturkbe
Copy link
Contributor Author

@bqpd I'm never going to get to finish this before I graduate. If you have a desire to continue this (for the sake of getting sensitivities to exponents), feel free to reuse the code. Otherwise, this can be closed for the near future.

@bqpd
Copy link
Contributor

bqpd commented Aug 2, 2021

@1ozturkbe no worries, makes sense! & let me know if I can do anything to help!

@bqpd
Copy link
Contributor

bqpd commented Aug 2, 2021

I'll leave it open just for visibility's sake...

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.

None yet

3 participants