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

⚡ Review fixes #2180

Merged
merged 1 commit into from
Jan 21, 2025
Merged

⚡ Review fixes #2180

merged 1 commit into from
Jan 21, 2025

Conversation

daquinteroflex
Copy link
Collaborator

@daquinteroflex daquinteroflex commented Jan 17, 2025

Re #2077 review.

  • Minor docstring reviews
  • Minimal MultiPhysicsMedium silicon library.
  • Silicon example default everywhere.
  • Debugged weird R parameter bug?

@daquinteroflex daquinteroflex mentioned this pull request Jan 17, 2025
@daquinteroflex daquinteroflex marked this pull request as ready for review January 17, 2025 15:58
Copy link
Contributor

@marc-flex marc-flex left a comment

Choose a reason for hiding this comment

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

Thanks @daquinteroflex for being on top of this. I'll finish the rest of the comments on Monday, when I'll probably have Daniil's feedback

@daquinteroflex
Copy link
Collaborator Author

daquinteroflex commented Jan 20, 2025

For the material library situation default, there are many combinations of a given Si SemiconductorMedium. This is due to the many recombination models, etc. There's one that makes most sense. I'm only going to create one combination of a MultiPhysicsMedium(charge=SemiconductorMedium(our single instantiated parameters)) in order to have a minimal instantiation, and the remaining silicon defaults I will make them a docstring example on each class since that way people can simply create the higher level models they want from them. They will have to read the docs to determine the corresponding defaults. Fyi @dbochkov-flexcompute

@dbochkov-flexcompute
Copy link
Contributor

For the material library situation default, there are many combinations of a given Si SemiconductorMedium. This is due to the many recombination models, etc. There's one that makes most sense. I'm only going to create one combination of a MultiPhysicsMedium(charge=SemiconductorMedium(our single instantiated parameters)) in order to have a minimal instantiation, and the remaining silicon defaults I will make them a docstring example on each class since that way people can simply create the higher level models they want from them. They will have to read the docs to determine the corresponding defaults. Fyi @dbochkov-flexcompute

looks great!

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.

Sorry this is getting nit-picky but some of the docstrings still sound unnecessarily verbose to me :)

@marc-flex marc-flex force-pushed the dario/charge/review_touches branch 2 times, most recently from f0f72a5 to f9dd9ba Compare January 21, 2025 11:32
@marc-flex marc-flex force-pushed the dario/charge/review_touches branch from b25c165 to e137a2a Compare January 21, 2025 12:33
@marc-flex
Copy link
Contributor

marc-flex commented Jan 21, 2025

Thanks everyone for your comments. I have addressed all of them.

I will merge this branch. Any additional comments can be dealt with in #2077

@marc-flex marc-flex merged commit f9270b7 into marc/dd-devsim Jan 21, 2025
@marc-flex marc-flex deleted the dario/charge/review_touches branch January 21, 2025 12:38
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.

5 participants