Skip to content
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

Fix: port computations for coarse grids #1558

Merged
merged 1 commit into from
Mar 19, 2024

Conversation

dmarek-flex
Copy link
Contributor

While testing a new example I found some issues with the way LumpedPorts are handled.

  • Small bug in port snapping that was missed when changing interface, now have improved test coverage.
  • For grids that do not line up exactly with LumpedPort bounds, the computation for current might choose the wrong bounding point.
  • For very coarse grids there might not be a single data point within the port bounds, so I added a method that checks this condition before the simulations are launched.

I did not modify the changelog, since these are all changes that could be considered within the original LumpedPorts PR.

@dmarek-flex dmarek-flex self-assigned this Mar 18, 2024
@dmarek-flex dmarek-flex added the Bug something isnt working label Mar 18, 2024
added grid size check to TerminalComponentModeler and improved test coverage of smatrix plugin
@dmarek-flex dmarek-flex marked this pull request as ready for review March 18, 2024 18:54
@dmarek-flex
Copy link
Contributor Author

BTW, keep in mind that the current and voltage computations are about to be changed in #1542, so they will be much simpler.

@@ -263,13 +276,21 @@ def port_current(port: LumpedPort, sim_data: SimulationData) -> xr.DataArray:
h1_field = h_field.isel({orth_component: orth_index - 1})
h2_field = h_field.isel({orth_component: orth_index})
h_field = h1_field - h2_field

# Find exact bounds of port taking into consideration the Yee grid
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a bit more explanation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to modify this computation anyways in #1542, and it should be much simpler.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this PR closed in favor of #1542. It seems the same change happening there already?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No I was planning on merging this PR soon, because it fixes some issue with coarse grids. Part of the issue was in this current computation. There is just a bit of overlap with #1542. I think the code in #1542 will be much more clear with the usage of the path integrals.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good. feel free to merge.

@dmarek-flex dmarek-flex merged commit 821bfb7 into pre/2.7 Mar 19, 2024
12 of 13 checks passed
@dmarek-flex dmarek-flex deleted the dmarek/fix_port_computations branch March 19, 2024 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug something isnt working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants