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

FEAT: Compute characteristic impedance from fields #1542

Merged
merged 1 commit into from
Apr 1, 2024

Conversation

dmarek-flex
Copy link
Contributor

@dmarek-flex dmarek-flex commented Mar 13, 2024

Addresses Issue 517. Most of computations are done, now it is just a question of the best way to expose this functionality to users. I wonder if @weiliangjin2021 or @tylerflex have any input on whether we should start a "microwave" plugin that includes functionality that might not be commonly used in photonics. I mention that because it would be very easy to add time domain Voltage and Current monitors now.

Some tasks remaining:

  • Update smatrix plugin to reuse path integrals
  • Add tests

@weiliangjin2021
Copy link
Collaborator

Agree that we should have a RF plugin. The other related issue is that we can have a wave port (which I think is the same as the current mode source, but slight difference in s-matrix parameter extraction?)

Copy link
Collaborator

@weiliangjin2021 weiliangjin2021 left a comment

Choose a reason for hiding this comment

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

Looks much cleaner now! In "path_integrals.py", there are duplicate codes in "normal_axis" and "remaining_axes". I wonder if you can define an abstract class that has those two methods, and then AxisAlignedPathIntegral and CurrentIntegralAA subclass from this class.

tidy3d/plugins/microwave/path_integrals.py Outdated Show resolved Hide resolved
tidy3d/plugins/microwave/path_integrals.py Outdated Show resolved Hide resolved
tidy3d/plugins/microwave/path_integrals.py Outdated Show resolved Hide resolved
tidy3d/plugins/microwave/path_integrals.py Outdated Show resolved Hide resolved
tidy3d/plugins/microwave/path_integrals.py Outdated Show resolved Hide resolved
@dmarek-flex dmarek-flex force-pushed the dmarek/impedance_from_fields branch 3 times, most recently from ea052ed to af968e4 Compare March 19, 2024 21:00
@dmarek-flex
Copy link
Contributor Author

I updated the changelog for this PR, but I was a little confused by the state of the versioning. It seems like LumpedElements have been added in 2.6, but that is not the case.

@weiliangjin2021
Copy link
Collaborator

I updated the changelog for this PR, but I was a little confused by the state of the versioning. It seems like LumpedElements have been added in 2.6, but that is not the case.

Yeah, it's messed up. I suppose it'll be cleaned up upon release. For now, you can edit near the part that should go pre/2.7. E.g. the Added section should go pre/2.7; There are 2 Changed section, the former one should go pre/2.7.

@dmarek-flex dmarek-flex marked this pull request as ready for review March 25, 2024 12:46
Copy link
Collaborator

@tylerflex tylerflex left a comment

Choose a reason for hiding this comment

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

Thanks @dmarek-flex ! Looks really good overall. This will be a great addition.

My comments are pretty minor but apply a bit globally at times so just make sure you address them not only where I commented. eg. the quotes, capitalization, and other formatting.

I do wonder if we need to build some more advanced DataArray handling for your applications into our main package, let me know if you have any thoughts on that.

tidy3d/plugins/microwave/path_integrals.py Outdated Show resolved Hide resolved
tidy3d/plugins/microwave/path_integrals.py Show resolved Hide resolved
tidy3d/plugins/microwave/path_integrals.py Outdated Show resolved Hide resolved
tidy3d/plugins/microwave/path_integrals.py Outdated Show resolved Hide resolved
tidy3d/plugins/microwave/path_integrals.py Outdated Show resolved Hide resolved
tidy3d/plugins/microwave/path_integrals.py Outdated Show resolved Hide resolved
tidy3d/plugins/microwave/path_integrals.py Outdated Show resolved Hide resolved
tidy3d/plugins/microwave/path_integrals.py Outdated Show resolved Hide resolved
tidy3d/plugins/microwave/path_integrals.py Outdated Show resolved Hide resolved
tidy3d/plugins/microwave/path_integrals.py Show resolved Hide resolved
@tylerflex
Copy link
Collaborator

@dmarek-flex will there be a notebook tutorial to go along with this?

@dmarek-flex
Copy link
Contributor Author

@dmarek-flex will there be a notebook tutorial to go along with this?

Yes, that would be a good idea. I have added an issue for the planned microwave notebooks to help with tracking.

@tylerflex tylerflex self-requested a review March 29, 2024 00:55
Copy link
Collaborator

@tylerflex tylerflex left a comment

Choose a reason for hiding this comment

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

Re: notebook will be good to have that as otherwise we've noticed users will seldom find these features on their own. Otherwise looks good to me!

…rent/impedance

reused path integrals in smatrix plugin
@dmarek-flex dmarek-flex merged commit 2797fe6 into pre/2.7 Apr 1, 2024
13 checks passed
@dmarek-flex dmarek-flex deleted the dmarek/impedance_from_fields branch April 1, 2024 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants