-
Notifications
You must be signed in to change notification settings - Fork 63
Add support for EME monitor data in outer_dot #2683
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: develop
Are you sure you want to change the base?
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.
Reviewing changes made in this pull request
Diff CoverageDiff: origin/develop...HEAD, staged and unstaged changes
Summary
tidy3d/components/data/monitor_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.
Thanks @caseyflex! Looks good overall but had some questions particularly about the _align_fields
method.
fields_1[key] = field_1 | ||
fields_2[key] = field_2 |
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.
Is it intentional that this function mutates the input? That seems potentially unsafe..
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 see. It's fine with this usage but agreed not good practice unless made explicit in the function signature.
def _align_fields( | ||
fields_1: dict[str, xr.DataArray], fields_2: dict[str, xr.DataArray], d_area: xr.DataArray | ||
) -> tuple[dict[str, xr.DataArray], dict[str, xr.DataArray], xr.DataArray]: |
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 left two comments but overall I find this function to be pretty difficult to follow. Maybe we could look at breaking the function down into smaller helpers to clarify the main workflow and better distinguish between the 'dot' and 'outer_dot' cases?
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 can try to clean this up, comment it, split it up.
In the end I didn't include this in dot
but maybe I can take another look at that.
Hi @caseyflex Casey, Thanks! I took a quick look at the code. It seems fine to me, but I’m sure Yaugenst will have much better insights than I do. I tried a simple example, using the taper from our EME notebook. I placed a yz EMEFieldMonitor and tried to do a mode overlap with a Gaussian beam, in the same way as our mode overlap example. The behavior seems the same as before: just calling eme_sim_data['field2'].outer_dot(gaussian_beam.field_data) returns the error: ValueError: operands could not be broadcast together with shapes (45,39,1,30,2) (45,39,1). Maybe I’m doing something wrong, or this isn’t the goal of the PR and I misunderstood? |
Previously, to take
outer_dot
with EME data, you had to select a singleeme_port_index
,eme_cell_index
,sweep_index
, etc. This had to be done one field component at a time and was a pain. This PR makesouter_dot
work directly with these additional fields, ensuring the input fields are aligned and broadcast appropriately. The additional fields end up as coords in the overlap DataArray.