Allow calling fix! twice in a row
#299
Conversation
src/variable.jl
Outdated
| @@ -127,8 +127,7 @@ end | |||
| fix!(x::Variable, v::Number) = fix!(x, fill(v, (1, 1))) | |||
|
|
|||
| function free!(x::Variable) | |||
| # TODO this won't work if :fixed appears other than at the end of x.sets | |||
| x.sets[end] == :fixed && pop!(x.sets) | |||
| deleteat!(x.sets, findall(==(:fixed), x.sets)) | |||
There was a problem hiding this comment.
I'm not super familiar with the fix!/free! stuff, but don't we only want to delete the last occurrence of :fixed?
There was a problem hiding this comment.
My understanding was :fixed should be basically a boolean toggle, either the variable is :fixed or it isn't: if it has :fixed in its sets field, then it should also have ConstVexity() and should be effectively treated like a constant. So if you fix!(x,3.0) and then fix!(x, 2.0), you are just changing the fixed value of x, but if you free!(x), you're saying it's just a regular variable again (and so it shouldn't have :fixed in it's .sets field anymore).
In fact, I think we don't need the concept of :fixed at all: for a Variable x, :fixed should be in x.sets if and only if x.vexity == ConstVexity() (and otherwise x.vexity == AffineVexity()). So we actually have two redundant ways to express x being fixed to a constant, and the bug that causes the segfault or NaN result with that test I added is what happens when these two concepts get out of sync: by calling fix! twice and then free!, the vexity gets reset to AffineVexity(), but it still has :fixed in its .sets field. Besides in fix! and free!, :fixed only shows up once in Convex.jl's source code, which is in conic_form!(x::Variable,...), which has the lines
if :fixed in x.sets
# do exactly what we would for a constant
...This causes the twice fix!'d then free!'d x to hit this code path despite having AffineVexity(). So in part of the problem formulation x is treated like a variable, but here it is treated like a constant. And I think that's what causes the bug that this PR's test checks.
I'll push another commit that actually removes :fixed altogether, so we just have 1 toggle to mean constant or not: ConstVexity() or AffineVexity(), which would hopefully remove the possibility for this kind of bug.
There was a problem hiding this comment.
So all the tests pass locally with this change (removing :fixed altogether). Maybe there is a usage of fix! and free! that is not covered by the tests (which are quite sparse regarding fix! and free!) which I don't understand where you don't want to remove all instances of :fixed though. I certainly don't want to be pushing for changes that will break people's code. To me, fixed or not as a boolean makes sense (either the variable is constant or it isn't), but maybe there is some more complicated usage.
However, I somewhat doubtful that there are lots of active users of fix! and free! currently, since until #294 (which hasn't made it into a release yet), the functionality was completely broken on any code hitting that path (e.g. #298). So me, it makes sense to take the opportunity to have a simple semantic (treated like a constant or treated like a variable; double fix!ing just updates the value of the constant, free!ing makes it a regular variable again).
There was a problem hiding this comment.
Thanks for the very detailed and thoughtful analysis here. I think you're right, doing everything in terms of the vexity instead of a Symbol-based toggle makes more sense. I feel a little uncomfortable accepting the change unilaterally only because I have so little knowledge of the fix!/free! stuff, but then again, not many people really look at this package nowadays...
There was a problem hiding this comment.
Thanks! Yeah, makes sense.
| @@ -127,8 +126,6 @@ end | |||
| fix!(x::Variable, v::Number) = fix!(x, fill(v, (1, 1))) | |||
|
|
|||
| function free!(x::Variable) | |||
| # TODO this won't work if :fixed appears other than at the end of x.sets | |||
| x.sets[end] == :fixed && pop!(x.sets) | |||
| x.vexity = AffineVexity() | |||
There was a problem hiding this comment.
This is mostly unrelated to the PR, but I noticed something: isn't it possible that x could have been something other than affine before fixing it as a constant? If that can occur, in such cases x would not actually have its vexity reset by freeing but rather changed entirely.
There was a problem hiding this comment.
I see what you mean, but I think Variable's only have ConstVexity() or AffineVexity(), where ConstVexity() corresponds to the fixed case. I think other vexities don't quite make sense for a Variable (but instead an expression involving a variable might have a different vexity, e.g. log(x) would be concave). To double check this, I searched the source for .vexity, since that would be the way to set a Variable to another vexity (since also searching for field and property doesn't show anything relevant), and only saw .vexity show up in a getter, in free!, in a show method, and in the line I changed,
if x.vexity == ConstVexity()
# do exactly what we would for a constant
...so I think indeed, Variable's vexities are not set to anything but AffineVexity() (and ConstVexity() in fix!).
There was a problem hiding this comment.
Just to say, I realized that actually I probably should have written vexity(x) instead of x.vexity in that if condition (we only need direct access for setting it, not accessing), so I pushed one (hopefully last) commit to change that.
|
Clearly no one else is going to look at this, so yolo |
* Allow calling `fix!` twice without a `free!` * Remove `:fixed` and just use `ConstVexity` * Use getter instead of direct field access
Currently, if you
fix!twice and thenfree!, you end up with a variable withAffineVexitybut:fixed, becausefree!only removes the last:fixedand still changes the convexity.On release and on master with ECOS, the test I added segfaults (which seems strange), and on release and master with SCS, the third test simply fails (the solver returns
NaN).