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

More options in DispersionFitter and fit constant loss tangent model #1652

Merged
merged 1 commit into from
May 3, 2024

Conversation

weiliangjin2021
Copy link
Collaborator

Addressing #1293 and use FastFitter to fit constant loss tangent material.

I'm not sure about the best way to implement #1293; currently it's using classmethod to load from complex permittivity and loss tangent, while we still only support wavelength but not frequency.

@weiliangjin2021
Copy link
Collaborator Author

One more comment: now that fitter is fast and stable, should we move material fitter from plugin to tidy3d components, so that we can do mat = AbstractMedium.constant_loss_tangent_model(eps_real, loss_tangent, frequency_range) as opposed to mat = FastDispersionFitter.constant_loss_tangent_model(eps_real, loss_tangent, frequency_range)?

@weiliangjin2021 weiliangjin2021 force-pushed the weiliang/loss_tangent branch 2 times, most recently from 6979bfe to d2cfb5e Compare April 30, 2024 21:33
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.

Implementation looks good to me!

Don't forget mention this in the changelog btw

tidy3d/components/medium.py Show resolved Hide resolved
tidy3d/plugins/dispersion/fit.py Outdated Show resolved Hide resolved
tidy3d/plugins/dispersion/fit.py Outdated Show resolved Hide resolved
tidy3d/plugins/dispersion/fit.py Outdated Show resolved Hide resolved
@tylerflex
Copy link
Collaborator

One more comment: now that fitter is fast and stable, should we move material fitter from plugin to tidy3d components, so that we can do mat = AbstractMedium.constant_loss_tangent_model(eps_real, loss_tangent, frequency_range) as opposed to mat = FastDispersionFitter.constant_loss_tangent_model(eps_real, loss_tangent, frequency_range)?

We can consider that, but we still need to support the plugin for backwards compatibility. So I guess is there a big advantage to moving it?

@weiliangjin2021
Copy link
Collaborator Author

One more comment: now that fitter is fast and stable, should we move material fitter from plugin to tidy3d components, so that we can do mat = AbstractMedium.constant_loss_tangent_model(eps_real, loss_tangent, frequency_range) as opposed to mat = FastDispersionFitter.constant_loss_tangent_model(eps_real, loss_tangent, frequency_range)?

We can consider that, but we still need to support the plugin for backwards compatibility. So I guess is there a big advantage to moving it?

We can leave a wrapper in the plugin for compatibility, similar to how we move complex polyslab from plugin to components:

from ...components.geometry.polyslab import ComplexPolySlabBase
from ...components.medium import MediumType
from ...components.structure import Structure
class ComplexPolySlab(ComplexPolySlabBase):

@tylerflex
Copy link
Collaborator

I see, if you find this would improve usability, feel free to change it. But at the same time, I'm fine with the way things are now too. Up to you

Copy link
Contributor

@dmarek-flex dmarek-flex left a comment

Choose a reason for hiding this comment

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

Looks good to me, excited to try some new applications.

tidy3d/plugins/dispersion/fit.py Outdated Show resolved Hide resolved
@weiliangjin2021
Copy link
Collaborator Author

@tylerflex any more comments?

@tylerflex
Copy link
Collaborator

no, looks good to me

@tylerflex tylerflex self-requested a review May 3, 2024 19:36
@weiliangjin2021 weiliangjin2021 merged commit 0262c0d into pre/2.7 May 3, 2024
16 checks passed
@weiliangjin2021 weiliangjin2021 deleted the weiliang/loss_tangent branch May 3, 2024 22:02
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.

None yet

3 participants