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

Convenience function to add charge perturbations with Nedeljkovic Soref Mashanovich model #2312

Merged
merged 1 commit into from
Apr 3, 2025

Conversation

marc-flex
Copy link
Contributor

@marc-flex marc-flex commented Mar 13, 2025

This PR adds a convenience class that allows to easily specify medium perturbation using the model in here. It addresses issue https://github.com/flexcompute/tidy3d-core/issues/846

For instance, in the Charge notebook one can now define the perturbation medium like this

perturbation_model = td.NedeljkovicSorefMashanovich(ref_freq=freq0)

si_perturb = td.PerturbationMedium.from_unperturbed(
    medium=si_non_perturb,
    perturbation_spec=td.IndexPerturbation(
        delta_n=perturbation_model.delta_n(),
        delta_k=perturbation_model.delta_k(),
        freq=freq0,
    )
)

The class NedeljkovicSorefMashanovich derives from a new class AbstractDeltaModel so potentially more perturbation models could be implemented following this approach (or used as template).

@marc-flex marc-flex self-assigned this Mar 13, 2025
@marc-flex marc-flex force-pushed the marc/perturbation-charge branch from ea65ba8 to effee69 Compare March 13, 2025 11:56
@marc-flex marc-flex marked this pull request as ready for review March 13, 2025 12:26
Copy link
Contributor

@dbochkov-flexcompute dbochkov-flexcompute left a comment

Choose a reason for hiding this comment

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

not sure what would be the best organization of this kind of model, given it so specific to a single material. I guess, one alternative I see is to simply have something like IndexPerturbation.from_nsm_si_model(f: float) class method instead of a new class. One concern, neither that nor the current implementation easily allow to include heat perturbations too

@marc-flex
Copy link
Contributor Author

I agree this model is very specific. That's why I made it derive from a more generic base class. This base class, in principle would allow to implement other models for other use cases. It would also allow users to create their own and use them with

def from_perturbation_delta_model(cls, deltas_model: AbstractDeltaModel) -> IndexPerturbation:
    """Create an IndexPerturbation from a DeltaPerturbationModel."""
   return IndexPerturbation(delta_n=deltas_model.delta_n, delta_k=deltas_model.delta_k)

Precisely because of it being so specific I'm not super keen on having something like IndexPerturbation.from_nsm_si_model(f: float). I'd prefer to have the specific functions somewhere else.

Copy link
Collaborator

@momchil-flex momchil-flex left a comment

Choose a reason for hiding this comment

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

I think I agree with @marc-flex here, @dbochkov-flexcompute if you find a quick moment to give final thoughts if we can merge this as is?

@dbochkov-flexcompute
Copy link
Contributor

yeah, sounds good!

@marc-flex marc-flex force-pushed the marc/perturbation-charge branch from 40fd6fe to 1c795a6 Compare April 3, 2025 08:13
@marc-flex marc-flex merged commit eeaa764 into develop Apr 3, 2025
18 checks passed
@marc-flex marc-flex deleted the marc/perturbation-charge branch April 3, 2025 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants