-
Notifications
You must be signed in to change notification settings - Fork 50
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
schema-backed postprocess option for invdes
#1828
Conversation
Thanks @tylerflex! Regarding the schema-defined FOM I have only two comments:
postprocess_obj = tdi.WeightedSum(
powers=[
tdi.GetPowerMode(monitor_name=MNT_NAME1, direction="+", mode_index=0),
tdi.GetPowerMode(monitor_name=MNT_NAME2, direction="+", mode_index=0)
],
weights=[0.3, 0.7]
) Regarding the interaction between the server-side optimizer and GUI:
|
Made things more general: introduced some abstract classes to separate custom, elementary, and combined postprocessing functions for extension later. changed naming:
Added two new ways to create an
|
Hi @e-g-melo , I just made some changes before seeing your comment. Let me address your questions though:
Yea I just changed it to
I thought about this, but then it's more to keep track of (weights, which index they refer to). It seemed easier to just stick it in the
we can do various things. There's a print of the most recent state, which we could parse. Or one can just take the result at each step of optimization and parse that. What would be easiest for you?
There are some optimizer methods, like
This is more tricky.. I dont even know how to do this for regular autograd at the moment. Is it possible to do a prototype without this? |
As per our Slack discussion, we need to implement a function similar to Another important feature is an |
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.
Looks great!
@@ -140,6 +140,13 @@ def post_process_fn(sim_data: td.SimulationData, **kwargs) -> float: | |||
return anp.sum(intensity.values) | |||
|
|||
|
|||
postprocess_obj = tdi.Sum( | |||
operations=[ |
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.
Wondering whether terms
would be more intuitive instead of operations
?
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.
the classes are also called Operations
so it kind of makes sense. But yea I wonder if there's an even better name for these?
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.
terms
sounds a bit strange to me.. it's more like elementary_operations
but that's too verbose.
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 don't know, terms
only really makes sense in the context of Sum
probably. Maybe it's fine as-is..
tidy3d/plugins/invdes/design.py
Outdated
@@ -11,11 +12,10 @@ | |||
from tidy3d.components.autograd import get_static | |||
|
|||
from .base import InvdesBaseModel | |||
from .postprocess import CustomPostprocessOperation, PostProcessFnType, PostprocessOperationType |
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.
We should decide for either PostProcess
or Postprocess
. My vote goes to PostProcess
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.
agree, replaced every Postprocess
with PostProcess
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 agree I prefer PostProcess
tidy3d/plugins/invdes/design.py
Outdated
@classmethod | ||
def from_function(cls, fn: PostProcessFnType, **kwargs) -> AbstractInverseDesign: | ||
"""Create an ``InverseDesign`` object from a user-supplied postprocessing function.""" | ||
|
||
postprocess = CustomPostprocessOperation.from_function(fn) | ||
return cls(postprocess=postprocess, **kwargs) |
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.
Does it really make sense to have InverseDesign
be able to construct a CustomPostProcessOperation
? Why does only postprocess
get this kind of special treatment?
I think there should not be multiple ways of constructing invdes objects from postprocess functions if those ways are functionally identical.
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 the idea was to make this
postprocess = CustomPostprocessOperation.from_function(fn)
des = InverseDesign(postprocess=postprocess, ...)
a bit simpler
des = InverseDesign.from_function(postprocess=postprocess, ...)
and save the user from having to know about CustomPostprocessOperation
.
The reason this gets special treatment I guess is that I think it will be used often enough (especially since we'll recommend users passing functions into Optimizer.run(f)
to switch to this). and the CustomPostprocessOperation
basically does nothing but be a class that the InverseDesign
schema recognizes.
What do you think?
tidy3d/plugins/invdes/postprocess.py
Outdated
return obj | ||
|
||
|
||
class ElementaryPostprocessOperation(AbstractPostprocessOperation, abc.ABC): |
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.
Does this really need to inherit from ABC
again?
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.
good question. I'm starting to doubt how I thought ABC classes worked. The idea though was that this ElementaryPostprocessOperation
is still abstract in the sense that we just use it for grouping / subclassing. We don't want the user to initialize one.
I thought that meant making it inherit directly from abc.ABC
but not I don't know. seems you can still initlize it. Have I been using ABC
wrong this whole time? when should we use it?
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 in principle this works:
class A(abc.ABC):
@abc.abstractmethod
def f(self):
...
class B(A):
def g(self):
...
b = B() # TypeError
However, if you use composition like, e.g., A(float, abc.ABC)
, then A
can be instantiated and is not abstract anymore. So yeah, since everything inherits from BaseModel
, I don't think any of our ABC
s really work as intended 😄
This can be solved through some fiddling with abc.ABCMeta
but I have to double check, forgot how exactly that works.
closing in favor of #1959 and #1848 but @yaugenst-flex just FYI if you want to see what I sketched out regarding these things in |
Currently it's almost the most simple thing imaginable while still being somewhat useful.
There's a
GetPowerMode
object that grabsModeData
from aSimulationData
, selects data, computes power, and sums with a weight.There's a
WeightedSum
that just sums each of these over a singleSimulationData
.This object can be added to
InverseDesign
, meaning that theOptimizer.run()
doesn't need to be passed a postprocess function.This makes it possible to do the following (assuming users use this approach):
Optimizer
to our server through web API and get aResult
back.Kind of a tentative working prototype, let me know what you think of the design / naming etc. Don't hold back any criticism, mostly for discussion.