invdes plugin GUI support
#1973
Conversation
| - **Named Variables**: For each named variable, you must provide a corresponding keyword argument. | ||
| If a named variable is missing during evaluation, a `ValueError` will be raised. | ||
|
|
||
| - **Multiple Unnamed Variables**: If your expression contains multiple unnamed variables, you cannot provide multiple positional arguments. |
There was a problem hiding this comment.
This detailed readme is great, thanks for putting it together. One comment here is just that this is making me wonder why bother support unnamed variables at all?
There was a problem hiding this comment.
it's not strictly necessary, but I thought it would be a nice convenience to not always have to provide keyword arguments, i.e. you might just want to do:
a = Variable()
f = 2 * a**2
f(2) # = 8if there were no unnamed variables, you'd have to do
a = Variable("a")
f = 2 * a**2
f(a=2) # = 8which is ever so slightly more cumbersome...
There was a problem hiding this comment.
Right, only now you have to remember the extra rule that you can only have one of those and stumble upon errors if you end up used to mixing them.. So in some sense just teaching people to always use the slightly more cumbersome version seems like less potential annoyances down the road.
There was a problem hiding this comment.
yeah fair enough, we can make the name required
There was a problem hiding this comment.
@momchil-flex this is actually a bit more involved than I thought initially, because we'd also require the metrics to be named (since they are also variables). and then there needs to be logic in the InverseDesign while constructing the objective function that extracts the names from potentially nested expression objects to pass the simulation data. like, I can add it, but it'll be a relatively big change to introduce now. I'm leaning towards leaving it as-is for now and implement that change if it turns out that the current approach is causing confusion?
it's a relatively small change w.r.t. what's user-facing.
There was a problem hiding this comment.
Yeah, ok, if it's that involved it's fine by me (even to potentially keep as is forever, it's a nit-y thing).
…n `continue_run`
yaugenst-flex
force-pushed
the
yaugenst-flex/invdes-gui
branch
from
8261c4c
to
78ee116
Compare
|
ok, well I guess we're probably ready to go here then? 😬 |
|
yea I think so! can be in a separate PR, but check out on slack the error I got using invdes today. might be related to an edge case in 2D sims that we should probably handle before we officially release this |
This is a rebased & cleaned-up version of #1848, with changes properly grouped. Got a bit messy with rebasing those commits (lots of overlapping changes introduced at different points of development), so figured this was cleaner.
Didn't want to force-push there in case we want to keep a reference to the original commits around.