-
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
Inverse Design Plugin #1560
Inverse Design Plugin #1560
Conversation
05ab7ca
to
a80d304
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see anything that looks out of place here. The user interface is pretty great! Definitely easier to use than going trough adjoint directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks great, should be very useful!
I see you've added the invdes.png image here and also in the notebooks PR, and it may not be used here?
One thing I noticed when I worked with the similar history handling that we have in our notebooks is that in some cases (esp. if I e.g. push things to large design regions like the 12x14um grating coupler I optimized) the memory needed to store the history can be quite large. This can make both the file large so it is inconvenient to handle (e.g. if you want to keep a bunch of optimization files they can take up a lot of space), and there's even a risk of running our of cpu memory completely since you store the whole history specifically for grad
, params
, and also the opt_state
which itself is of a similar or larger size to params
. Along those lines I feel like some more control over what exactly is stored, and some good default settings, may provide for a better experience?
For example, I probably always want to store the history of the objective_fn_val
, penalty
, and post_process_val
. In most cases I probably just want the last params
and opt_state
so that I can examine my last design and continue an optimization. I probably almost never actually need to look at the grad
. Now, I realize that using these as defaults (do not store history for params
, grad
, opt_state
) limits functionality a bit (e.g. if I want to make a gif of the params evolution, or if I want to start from a state that's not the latest one). But if we allow the option to enable those, do you think it may be better?
tidy3d/plugins/invdes/optimizer.py
Outdated
params = jnp.maximum(params, 0.0) | ||
|
||
history["params"].append(params) | ||
history["opt_state"].append(opt_state) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I always found this tricky previously when we wrote similar optimization history scripts. Here it seems like the params that you write do not match all the other values that you have written in history, i.e. the grad, objective function, etc. were computed for the params before the update, while here you write them to history after the update. You can certainly move history["params"].append(params)
to be above with the other history updates but I think there was some reason to put it later, maybe related to what happens when you continue an optimization? I remember I thought about this once and it was not as straightforward as one would expect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what I did is when the optimizer is run from an empty state Optimizer.run(params)
, it will store the initial params
and opt_state
before running this loop.
tidy3d/tidy3d/plugins/invdes/optimizer.py
Line 76 in 1844968
return InverseDesignResult(design=self.design, opt_state=[opt_state], params=[params0]) |
So actually for num_steps
steps, we end up having num_steps+1
values for params
and opt_state
in the history but only num_steps
values for the other stuff.
It's not always clear whether people want the final state / parameters to be after the final update or what. But I decided to include it here just for completeness. And also so that if we continue a run it will not waste that gradient calculation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think maybe not wasting the gradient computation is what was tripping me up before too. But if I understand correctly this can definitely be confusing now? First of all having history
contain lists of different lengths is already not great. But probably the biggest problem is that if someone does result.get_last("objective_fn_val")
, this value will not correspond to the simulation I get from result.get_sim()
, if I then do a manual run on this and compute the objective. This seems like something that would happen often.
One is used for the notebook itself, and the other is used in the README.md for
A few things:
I think changing the fields in the
history = dict(params=[], ...)
def callback(current_results, step_index: int) -> None:
history['params'].append(params)
optimizer = Optimizer(..., callback=callback)
optimizer.run(params0)
history['params']
def postprocess(sim_data, ...):
...
return objective, dict(data=sim_data)
history = dict(data=[])
def callback(..., aux_data):
history['data'].append(aux_data['data'])
I think the callback / aux_data could be good enough to support this, but the learning curve is a bit high. What do you think? |
You mean the history saving to file? That's true, but it's nice to have. And apart from that it's always stored in memory currently.
Yeah but you only need the last value, not the whole history. My suggestion was to store only the last value by default.
Storing
This does get more complicated. Maybe for starters we leave as is. But I was more driving towards something like an e.g. |
@momchil-flex sounds good for all points, I'll play around with a I think more complicated callbacks can be added later, including for more customized history storage, display, and parameter scheduling. |
@momchil-flex I implemented something in this commit. Could you take a look when convenient? thanks. |
4259c4e
to
1365701
Compare
0b958db
to
2422bab
Compare
Notebook PR:
flexcompute/tidy3d-notebooks#94
To Do list as of Mar 26
pd.Field
descriptions