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

Medium perturbation #996

Merged
merged 1 commit into from
Jul 28, 2023
Merged

Conversation

dbochkov-flexcompute
Copy link
Contributor

@dbochkov-flexcompute dbochkov-flexcompute commented Jul 12, 2023

Heat and charge perturbation functionality. Currently, implemented for Medium and PoleResidue, but this is ready for an initial "big picture" review, to check if anything seems bad/needs a rework. Here's a notebook with demonstrations and explanations to help the review https://github.com/flexcompute-readthedocs/tidy3d-docs/blob/daniil/perturbation-medium/docs/source/notebooks/PerturbationMedium.ipynb. If everything looks more or less alright, I will go ahead and finish some remaining to-do's:

  • do the rest of dispersive models, that is, add PerturbationDebye, PerturbationLorentz, etc
  • add front-end tests
  • check all code documentation is in place

tagging @momchil-flex just in case

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 Daniil, this is a ton of work and I think it is really well organized and covers everything I can think of very well. Most of my comments are just refining it for public consumption. The notebook is also extremely useful and will be invaluable. There were a few spelling issues, I recommend this https://pypi.org/project/jupyterlab-spellchecker/ to catch some of them before we do a more thorough proofread.

Thanks!

tidy3d/components/medium.py Outdated Show resolved Hide resolved
tidy3d/components/medium.py Outdated Show resolved Hide resolved
tidy3d/components/medium.py Outdated Show resolved Hide resolved
tidy3d/components/parameter_perturbation.py Outdated Show resolved Hide resolved
tidy3d/components/parameter_perturbation.py Outdated Show resolved Hide resolved
tidy3d/components/simulation.py Outdated Show resolved Hide resolved
tidy3d/components/simulation.py Outdated Show resolved Hide resolved
tidy3d/components/simulation.py Outdated Show resolved Hide resolved
tidy3d/components/simulation.py Show resolved Hide resolved
tidy3d/plugins/dispersion/fit.py Outdated Show resolved Hide resolved
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.

Overall looks quite good! A few small comments.

tidy3d/components/data/data_array.py Show resolved Hide resolved
tidy3d/components/parameter_perturbation.py Show resolved Hide resolved
tidy3d/components/parameter_perturbation.py Show resolved Hide resolved
tidy3d/components/parameter_perturbation.py Outdated Show resolved Hide resolved
tidy3d/components/parameter_perturbation.py Outdated Show resolved Hide resolved
@dbochkov-flexcompute
Copy link
Contributor Author

big cleanup, added many docstrings, added tests, and fixed many loose ends. I am considering adding rest of dispersive models PerturbationDebye, PerturbationLorentz, etc, later, and merging this without them. What do you think?

@dbochkov-flexcompute dbochkov-flexcompute changed the base branch from develop to pre/2.4 July 18, 2023 23:31
@dbochkov-flexcompute dbochkov-flexcompute marked this pull request as ready for review July 18, 2023 23:31
tidy3d/components/validators.py Outdated Show resolved Hide resolved
allowed_imag_range=[None],
allowed_complex=False,
)
_conductivity_perturbation_validator = validate_parameter_perturbation(
Copy link
Collaborator

Choose a reason for hiding this comment

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

If allow_gain=True, conductivity can be < 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so, these validators only warn if conductivity/permittivity can potentially go out of bounds (within the provided/calculated temperature_range, electron_range, and hole_range). The strict validation is left to the corresponding CustomMedium. So perhaps it's still ok to warn the user about potentially negative conductivity even if allow_gain=True?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, so it will warn and then error. In that regard, there seems to be missing allow_gain in the return of

    return CustomMedium(
            permittivity=permittivity_field,
            conductivity=conductivity_field,
            name=self.name,
            subpixel=True,
        )

and it seems better to add subpixel as a field of PerturbedMedium so that one can choose to apply subpixel or not. But warn that for a uniform medium, supixel parameter will be ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, you're right, thanks for catching that. Actually, frequency_range should also be passed. Fixed it.

@weiliangjin2021
Copy link
Collaborator

big cleanup, added many docstrings, added tests, and fixed many loose ends. I am considering adding rest of dispersive models PerturbationDebye, PerturbationLorentz, etc, later, and merging this without them. What do you think?

Agree, as long as we can have a good heat example in the current stage.

@tylerflex tylerflex added the 2.4 label Jul 21, 2023
@dbochkov-flexcompute dbochkov-flexcompute force-pushed the daniil/medium-perturbation branch 2 times, most recently from 9ec7e3a to b448927 Compare July 28, 2023 19:30
@dbochkov-flexcompute
Copy link
Contributor Author

if there are no other comments at this point, is it ok to merge this into pre/2.4?

@tylerflex
Copy link
Collaborator

Yea I've given all of my comments. I think it's ok if everyone else does

@weiliangjin2021
Copy link
Collaborator

Green light from me.

@dbochkov-flexcompute dbochkov-flexcompute merged commit 4710a1c into pre/2.4 Jul 28, 2023
11 checks passed
@dbochkov-flexcompute dbochkov-flexcompute deleted the daniil/medium-perturbation branch September 5, 2023 23:59
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