-
Notifications
You must be signed in to change notification settings - Fork 40
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
Vector substitutions for vectorized models #1522
Comments
Just want to say, this is a really well-written issue. Assuming this prompts a code change, let's definitely add these MWE's as a unit test. |
Thanks @whoburg! |
For the sake of completeness (and for any users experiencing this issue in the future), it's worth including two workarounds in case this doesn't prompt a code change. Option 1: Direct substitutionDownside: doesn't address post-model-creation substitutions import numpy as np
from gpkit import Variable, Model, ConstraintSet, Vectorize
class Pie(Model):
def setup(self):
# self.x = x = Variable("x") <-- before
self.x = x = Variable("x", [[2], [3]])
z = Variable("z")
constraints = [
x >= z,
]
substitutions = {'z': 1}
return constraints, substitutions
class Cake(Model):
def setup(self):
self.y = y = Variable("y")
with Vectorize(2):
s = Pie()
constraints = [y >= s.x]
constraints += [s]
#subs = {'x': np.array([[2], [3]])} <-- before
#return constraints, subs <-- before
return constraints
class Yum2(Model):
def setup(self):
with Vectorize(2):
cake = Cake()
y = cake.y
self.cost = sum(y)
constraints = ConstraintSet([cake])
return constraints
m = Yum2()
sol = m.solve()
print(sol.table()) Option 2: numpy.tileDownside: requires sub-model-level knowledge of its own vectorization. Also, gross. import numpy as np
from gpkit import Variable, Model, ConstraintSet, Vectorize
class Pie(Model):
def setup(self):
self.x = x = Variable("x")
z = Variable("z")
constraints = [
x >= z,
]
substitutions = {'z': 1}
return constraints, substitutions
class Cake(Model):
def setup(self):
self.y = y = Variable("y")
with Vectorize(2):
s = Pie()
constraints = [y >= s.x]
constraints += [s]
#subs = {'x': np.array([[2], [3]])} <-- before
numberofcakes = Vectorize.vectorization[0]
subs = {'x': np.tile(np.array([[2], [3]]), numberofcakes)}
return constraints, subs
class Yum2(Model):
def setup(self):
with Vectorize(2):
cake = Cake()
y = cake.y
self.cost = sum(y)
constraints = ConstraintSet([cake])
return constraints
m = Yum2()
sol = m.solve()
print(sol.table()) |
Right now the cleanest fix seems to me to be vectorizing that substitution upon finishing Cake's |
in re: workarounds, option 1 is very fragile; I would recommend using option 2 (with a big |
@pgkirsch I'm thinking of implementing the fix mentioned two comments above; does that seem like it addresses your issue? |
If that seems like the cleanest solution to you -- and it doesn't feel like too much of a hack to appease a maybe-weird use-case -- then that sounds good to me! I guess I was expecting the change to be to |
@pgkirsch looking at this again, I there's an Option 3: you don't have to propagate the number of cakes (as in your Option 2 above), because that information is already encoded in xsub = np.broadcast_to([2, 3], reversed(s.x.shape)).T
subs = {'x': xsub} it works for any number of cakes. This is obviously a bit clunky. The alternative is this being added to CostedConstraintSet.__init__(self, cost, constraints)
if substitutions: # check if we have to broadcast any vecsubs
for key, value in substitutions.items():
try:
vk, idx = self.substitutions.parse_and_index(key)
if not idx and vk.shape != getattr(value, "shape", None):
substitutions[key] = np.broadcast_to(value,
reversed(vk.shape)).T
except KeyError: # didn't parse, presume fine
pass
self.substitutions.update(substitutions) replacing This code adds more surface area for bugs...but I really like being able to say |
I tried running tests with the modification and found it interacted with boundedness tests in a negative way :( Could be fixed by moving that part of ConstraintSet into a method, but this leans me further towards not implementing this for returned substitutions dictionaries. |
Thanks for looking into this more @bqpd! I don't want to break other functionality with this (for now) very uncommon use-case. That Use would be:
Though I'm sure there's probably a more elegant solution that doesn't involve redundant |
yeah that's a great idea! called it |
(Forgive me, I know this has been discussed before but I can't find a ticket that answers my question)
When a scalar substitution is made in a vectorized model, it seems that substitution is "tiled" for any arbitrary model vectorization.
But when a multi-element substitution is made for a vectorized variable, it only seems to work as long as the parent model is not vectorized.
Here is a MWE of something that works:
Here is a MWE of something that breaks:
The only difference is that Cake is vectorized in Yum2. The workaround is to use np.tile to make the substitution the correct shape (using the global vectorization) but that's inelegant (in the non-toy version of my problem) and I'd like to move away from it if at all possible.
Is there code that correctly tiles scalars but not vectors or am I thinking about it wrong? Is there something ambiguous about the substitutions I'm trying to make that would prevent this from being possible?
The text was updated successfully, but these errors were encountered: