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

Added PECMedium to IsotropicUniformMediumType, and added PEC2D #1193

Merged
merged 1 commit into from
Nov 10, 2023

Conversation

caseyflex
Copy link
Contributor

No description provided.

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.

seems ok to me, do we need to add a changelog item in "fixed"?

Asking @shash-sharma for a review too since I'm not totally following the original problem this is fixing.

@tylerflex tylerflex added the 2.5 label Oct 12, 2023
@momchil-flex
Copy link
Collaborator

Sorry, what is the status of this? @shash-sharma ?

@shashwat-sh
Copy link
Contributor

I think we're waiting on PEC subpixel, and some revisions on this and the associated backend PR

@momchil-flex
Copy link
Collaborator

PEC subpixel will almost certainly not be in 2.5 - are you saying this and the backend PRs should then also not be labeled as 2.5?

@caseyflex
Copy link
Contributor Author

caseyflex commented Oct 20, 2023 via email

@caseyflex
Copy link
Contributor Author

caseyflex commented Oct 20, 2023 via email

@caseyflex
Copy link
Contributor Author

@weiliangjin2021 do you want to change the meshing to use the relevant component even when another component is PEC?

@weiliangjin2021
Copy link
Collaborator

@weiliangjin2021 do you want to change the meshing to use the relevant component even when another component is PEC?

Could you edit is_comp_pec method in Medium2D, and push the changes directly? @caseyflex

@weiliangjin2021 weiliangjin2021 marked this pull request as ready for review November 8, 2023 17:43
@weiliangjin2021
Copy link
Collaborator

One caveat in scene visualization is that only an isotropic PEC will be plotted as PEC. An anisotropic medium where some components are PEC is plotted as regular medium. Do you think it's fine @tylerflex ?

@caseyflex
Copy link
Contributor Author

@weiliangjin2021 do you want to change the meshing to use the relevant component even when another component is PEC?

Could you edit is_comp_pec method in Medium2D, and push the changes directly? @caseyflex

It seems like is_comp_pec should only be a method in AnisotropicMedium, since it doesn't make sense for an arbitrary AbstractMedium. Then, we can have is_comp_pec in Medium2D as well, but in addition to taking the component axis, it can take the normal axis, which is known in the mesher. This would be required to map ss, tt to xx, yy, zz.

Copy link
Contributor

@shashwat-sh shashwat-sh left a comment

Choose a reason for hiding this comment

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

This makes sense to me. About the plotting, I think the current behaviour of only plotting isotropic PEC as PEC makes sense, since the anisotropic case with some PEC components seems to be a fairly special case, and probably used in a very different context.

@caseyflex
Copy link
Contributor Author

@weiliangjin2021 , I pushed the changes we discussed. Also, I changed it to plot as PEC if any component is PEC, rather than if all components are PEC. Please check if this new handling makes sense.

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.

One last comment. Otherwise all look good to me.

tidy3d/components/grid/mesher.py Show resolved Hide resolved
@caseyflex
Copy link
Contributor Author

One last comment. Otherwise all look good to me.

thanks for the catch. This should be ready, maybe we want to squash the commits though.

@weiliangjin2021
Copy link
Collaborator

One last comment. Otherwise all look good to me.

thanks for the catch. This should be ready, maybe we want to squash the commits though.

Thanks, I'll take care of that. Usually we squash just before the merge so that changes can be better monitored.

@momchil-flex
Copy link
Collaborator

So is there anything left to do @weiliangjin2021 ?

@weiliangjin2021
Copy link
Collaborator

So is there anything left to do @weiliangjin2021 ?

just start to squash and merge.

@weiliangjin2021 weiliangjin2021 merged commit f7edada into pre/2.5 Nov 10, 2023
12 checks passed
@weiliangjin2021 weiliangjin2021 deleted the casey/pec2d branch November 10, 2023 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rc3 3rd pre-release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants