-
Notifications
You must be signed in to change notification settings - Fork 63
feat: RF GUI <-> python client
interoperabilty
#2663
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
base: dario/release-2.9.0
Are you sure you want to change the base?
Conversation
RF GUI<->python client
interoperabilty
RF GUI<->python client
interoperabiltyRF GUI <-> python client
interoperabilty
0e00e0d
to
49f3708
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.
Reviewing changes made in this pull request
The failing tests is due to the random data, which was previously circumvented using monkeypatch to ignore the check. To fix the breaking tests, you need to update this monkeypatch part with something that will ignore that check. |
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.
7 files reviewed, 3 comments
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.
8 files reviewed, 2 comments
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.
18 files reviewed, no comments
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.
18 files reviewed, 1 comment
Returns | ||
------- | ||
folders : [Folder] | ||
List of folders |
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.
style: Documentation inconsistent with type hint. Return type shows '[]' but docstring indicates '[Folder]'
Returns | |
------- | |
folders : [Folder] | |
List of folders | |
def list(cls, projects_endpoint: str = "tidy3d/projects") -> list[Folder]: |
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.
Thanks @daquinteroflex! Left a couple of comments but overall I would say if it works we'll go with it. Depends a bit on how many breaking API changes we can introduce here. In general I would probably try to put many more methods into the data model. In some sense these are pretty similar to the monitor data in components, which also do a lot of domain-specific postprocessing.
# BREAK this test because it introduces mutability which shouldn't exist | ||
# assert modeler2.batch_cached == modeler2.batch == batch |
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.
But that seems to be a problem with the implementation and not with the test...?
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.
Sure maybe it's probably the type hint that can fix it when it rehydrates
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.
Renabled and passing
@@ -168,8 +174,10 @@ def to_file(self, fname: str) -> None: | |||
super(AbstractComponentModeler, self).to_file(fname=fname) # noqa: UP008 |
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 noqa
here should go, as should the parameters to super()
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 I think a lot of this stuff is from the privous implementation
@staticmethod | ||
def _set_port_data_array_attributes(data_array: PortDataArray) -> PortDataArray: | ||
"""Helper to set additional metadata for ``PortDataArray``.""" | ||
data_array.name = "Z0" | ||
return data_array.assign_attrs(units=OHM, long_name="characteristic impedance") |
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.
Ooof.. is this really necessary? Why does the array need to be mutated?
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.
This is from my original implementation. My reasoning was that there are many types of data that can be represented by the same DataArray
types, since they have the same dimensions or coordinates.
Is it better to create a new DataArray
type just for impedance values, or change this method to copy and modify the input?
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. In principle you're right of course but this is exactly the reason why we have so many different array types. We'll have to rethink that at some point but I think for now it'd be better to just introduce a separate type.
tidy3d/plugins/smatrix/local_run.py
Outdated
) | ||
|
||
|
||
def construct_smatrix( |
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.
This seems like it should be a method of ComponentModelerData
. Actually this is my impression for most of the functions in here.
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 I was thinking it'd be good if we can separate the "data construction" methods from the data containers and the simualtion definitions. ie. it'd be nice if they're self contained functions that just take in local.run(ComponentModeler) -> ComponentModelerData
to avoid the mixing up of stuff in the original implementation. I've moved them to a separate file because this way we can reuse them for backwards and onwards compatibilty without having to recreate a TerminalComponentModelerData
class inside TerminalComponentModeler
to use this method directly for now.
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.
Also would have expected most of these methods to be part of ComponentModelerData
, since that is how these things are mostly organized for MonitorData
. I don't have a strong preference either way. But shouldn't you also move get_antenna_metrics_data
and _monitor_data_at_port_amplitude
to this file as well then?
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.
Do we have a general guideline on those separations that are (or will be) applied to the entire tidy3d?
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.
Will reply privately to both this and this, as related to future plans as far as I understand them
Also would have expected most of these methods to be part of ComponentModelerData, since that is how these things are mostly organized for MonitorData. I don't have a strong preference either way. But shouldn't you also move get_antenna_metrics_data and _monitor_data_at_port_amplitude to this file as well then?
tidy3d/plugins/smatrix/utils.py
Outdated
return z_matrix | ||
|
||
|
||
def check_port_impedance_sign(Z_numpy: np.ndarray): |
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.
This seems like it should be a validator somewhere.
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.
Agreed, the problem is backwards compatibility on how this method gets used dynamically in construct_smatrix
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.
When a WavePort
is present, the port impedance is calculated in a post-processing step. Does it still make sense to make this a validator?
@@ -37,15 +37,15 @@ class Folder(Tidy3DResource, Queryable, extra=Extra.allow): | |||
) | |||
|
|||
@classmethod | |||
def list(cls) -> []: | |||
def list(cls, projects_endpoint: str = "tidy3d/projects") -> []: |
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.
Return type hint should be list
, not []
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 endpoints code itself has not been finished because it depends on whatver implementation we choose to do, so should have clarfied this is still WIP or can remove it for now
tidy3d/plugins/smatrix/data/data.py
Outdated
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.
rename modeler_data.py
?
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.
Just made a first pass, and the basic idea looks good to me. My main uncertainty is the the usage of the post-processing functionality present in local_run.py
. Ignoring backwards compatibility, can you explain why that would be better than having them as members in the TerminalComponentModelerData?
tidy3d/plugins/smatrix/data/data.py
Outdated
"Stores the computed S-matrix and reference impedances for the terminal ports" | ||
from tidy3d.plugins.smatrix.local_run import construct_smatrix | ||
|
||
terminal_port_data = construct_smatrix(simulation=self.simulation) |
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.
Think it would be better to directly use self.data
now
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.
But I also see the problem, you would need to add a method for going from dict[TerminalPortType, SimulationData]
to dict[str, SimulationData]
which would be compatible to how the batch_data is used.
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 aiming for somthing lke
sim_data = modeler_data.data[port_in]
and/or
def compose_terminal_modeler_data(
modeler: TerminalComponentModeler,
batch_data: BatchData = None,
) -> TerminalComponentModelerData:
port_to_sim_data_map = {
port_i: batch_data[modeler.get_task_name(port=port_i)] for port_i in modeler.ports
}
port_simulation_data = PortSimulationData(data=port_to_sim_data_map)
return TerminalComponentModelerData(modeler=modeler, data=port_simulation_data)
tidy3d/plugins/smatrix/utils.py
Outdated
return z_matrix | ||
|
||
|
||
def check_port_impedance_sign(Z_numpy: np.ndarray): |
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.
When a WavePort
is present, the port impedance is calculated in a post-processing step. Does it still make sense to make this a validator?
@@ -35,6 +35,11 @@ | |||
class AbstractComponentModeler(ABC, Tidy3dBaseModel): | |||
"""Tool for modeling devices and computing port parameters.""" | |||
|
|||
name: str = pd.Field( |
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.
Just needs update for title and description
tidy3d/plugins/smatrix/data/data.py
Outdated
description="Reference impedance for each port used in the S-parameter calculation. " | ||
"This is optional and may not be present if not specified or computed.", | ||
) | ||
data: TerminalPortDataArray = pd.Field( |
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.
rename to s
tidy3d/plugins/smatrix/data/data.py
Outdated
class MicrowavePortSimulationData(Tidy3dBaseModel): | ||
"""Stores raw simulation data from each microwave port-specific simulation.""" | ||
|
||
data: dict[TerminalPortType, SimulationData] = pd.Field( |
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 think this should change to dict[str, SimulationData]
for two reasons:
- One to make it easier to take the place of
batch_data
in some of these helper functions for postprocessing. Since I think in every post processing function thebatch_data
is mainly used as a mapping from a task name to the simulation data. - More importantly, a single
Port
may be associated with more than oneSimulationData
, like when we support multimodalWavePort
where each mode will correspond with a different simulation.
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 how does the current implementation of sound like? Achieves that goal
PortReferenceType = Union[str, TerminalPortType] # TODO Debate this
class PortSimulationData(Tidy3dBaseModel): # TODO Debate typing.TypedDict instead, I prefer
"""Stores raw simulation data from each microwave port-specific simulation."""
data: dict[PortReferenceType, SimulationData] = pd.Field(
tidy3d/plugins/smatrix/local_run.py
Outdated
) | ||
|
||
|
||
def construct_smatrix( |
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.
Also would have expected most of these methods to be part of ComponentModelerData
, since that is how these things are mostly organized for MonitorData
. I don't have a strong preference either way. But shouldn't you also move get_antenna_metrics_data
and _monitor_data_at_port_amplitude
to this file as well then?
69b2f32
to
a676b07
Compare
a676b07
to
9e89977
Compare
Not particularly pleased with this implementation, but can't think of a cleaner option whilst mantaining backwards compatiblity. We should properly design this though and looking forward to the webapi refactor one day.
There are more things that maybe would be worth changing, but unless we truly break backwards compatibility and have to update several notebooks... Maybe that is TBC in a different PR, for now this is just about standardising the simulation and data flow.
So ultimately, I think this should be close to achieving the goal of having a self-contained
TerminalComponentModelerData
as a serializable and deserializable data structure. However, again, unless we break backwards compatibility withbatch_data
onxComponentModeler
, we cannot have fully pure functions. However, I have tried to at least "purify" the batch_data usage wherever it is called so that we can replace it in the future fromsimulation.batch_data
.Open to chatting about the exact data structures chosen for the SMatrixData types, it was mainly done by chatting with Damian and enabling backwards compatibility. It was renamed
MicrowaveXData
to reduce the scope and compatibility of these changes, really we should revisit this with a holistic view and willing to break compatibility. However, having these strong data types should in any case support the migration effort whatever we do onwards.Considerations:
web.run(TerminalComponentModeler) -> TerminalComponentModelerData
, by using alocal_run.run(TerminalComponentModeler) -> TerminalComponentModelerData
Again, I'm not super happy with it, but feel it's a bit better and can help us move forward. Open to suggestions to improve or feel free to propose changes directly.
TODO
batch_cached
is a mutation aberration that we should remove sooner than later.Greptile Summary
This PR implements interoperability between the RF GUI and Python client for Terminal Component Modeling through a significant architectural restructuring. The changes focus on standardizing data flow and S-matrix computations while maintaining backward compatibility with existing implementations.
Key changes include:
Confidence score: 2/5