-
Notifications
You must be signed in to change notification settings - Fork 66
Reorganizing of ModeSolver and ModeSolverData #260
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
Conversation
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.
Approved pending some discussion around the really minor comments I have.
tidy3d/plugins/mode/mode_solver.py
Outdated
---------- | ||
mode_solver : :class:`.ModeSolver` | ||
Original mode solver instance. | ||
data_dict : Dict[str, :class:`.AbstractModeData`] |
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.
maybe here we just put the same type as line 44 with the union.
"mode_spec": self.mode_spec, | ||
} | ||
for name, obj in json_dict.items(): | ||
Tidy3dData.save_string(handle, name, obj.json()) |
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.
note this is not storing 'Infinity' as a string.
"mode_spec": ModeSpec, | ||
} | ||
obj_dict = {} | ||
for name, obj in json_dict.items(): |
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.
clever ;)
ModeSource | ||
Modal source containing specification in ``mode``. | ||
:class:`.ModeSource` | ||
Mode source with specifications taken from the ModeSolver instance and the method |
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.
:class: on modeSolver
---------- | ||
mode_spec : :class:`ModeSpec` | ||
:class:`ModeSpec` object containing specifications of mode. | ||
freqs : List[float] |
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.
these freqs are all stored in the ModeSolver? So do we want to:
a) keep it the way it is
b) use self.freqs
here?
b) use self.freqs
if freqs is None
or something fancier like this?
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 kinda like keeping it the way it is because it makes sure the user knows what's happening and passes the frequencies they want, or in general knows that it's possible to export a monitor with different (e.g. more) frequencies that they used in the solver. So I'm between a) or c), but like a).
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.
ok let's keep it (a) then, that's fine with me.
ModeMonitor | ||
Monitor that measures modes specified by ``mode_spec`` on ``plane`` at ``freqs``. | ||
:class:`.ModeMonitor` | ||
Mode monitor with specifications taken from the ModeSolver instance and the method |
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.
:class:ModeSolver
Some shared code with SimulationData w.r.t. plotting fields SimulationData.plot_field now doesn't need x/y/z/ for a 2D monitor
Notes: