-
Notifications
You must be signed in to change notification settings - Fork 1.1k
add capability for reverse bias #948
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
Conversation
Ready for review. stickler complains about breaks both before and after an operator. Suggestions? Test failures are for |
d2mutau=0, NsVbi=np.Inf, method='newton'): | ||
d2mutau=0, NsVbi=np.Inf, breakdown_factor=0., | ||
breakdown_voltage=-5.5, breakdown_exp=3.28, | ||
method='newton'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a convention, is it better to add new kwargs at the end, E.g. after method='newton'
, so it doesn't break code that relies on position than explicit kwarg=
usage?
I'm half asking if that's something that's been historically done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but would like to keep like arguments grouped together in general order of decreasing interest: required module parameters first, then optional thin-film parameters, reverse bias parameters third, numerical control last.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's bad form to rely on the position of a keyword argument. Don't worry about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM other than a few NITs and linting.
Co-Authored-By: Cameron Stark <CameronTStark@users.noreply.github.com>
Co-Authored-By: Cameron Stark <CameronTStark@users.noreply.github.com>
Co-Authored-By: Cameron Stark <CameronTStark@users.noreply.github.com>
d2mutau=0, NsVbi=np.Inf, method='newton'): | ||
d2mutau=0, NsVbi=np.Inf, breakdown_factor=0., | ||
breakdown_voltage=-5.5, breakdown_exp=3.28, | ||
method='newton'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's bad form to rely on the position of a keyword argument. Don't worry about it.
Thanks @cwhanse! |
docs/sphinx/source/api.rst
for API changes.docs/sphinx/source/whatsnew
for all changes. Includes link to the GitHub Issue with:issue:`num`
or this Pull Request with:pull:`num`
. Includes contributor name and/or GitHub username (link with:ghuser:`user`
).Adds reverse bias capability to single diode functions. The Lambert W technique cannot solve the single diode equation with the term for current at reverse bias. The Bishop '88 methods can solve the equation. Tested with
method='newton'
, not clear ifmethod='brentq'
is reliable for this application.