Fix ZeroDivisionError in simple_efficiency when load_loss=0 #2646
Conversation
pvlib/transformer.py
Outdated
| if load_loss == 0: | ||
| return input_power - no_load_loss * transformer_rating | ||
|
|
There was a problem hiding this comment.
| if load_loss == 0: | |
| return input_power - no_load_loss * transformer_rating |
I don't think this is necessary with the alternate form of the quadratic equation.
There was a problem hiding this comment.
i have removed it
There was a problem hiding this comment.
Could you add a test with load_loss=0?
There was a problem hiding this comment.
thanks!! @cwhanse
i have added a test covering the load_loss=0 that checks the linear-limit behavior P_out = P_in − L_no_load * P_nom
please let me know if you would prefer this test to also cover array like inputs or assert behavior at additional edge cases (negative input power)
There was a problem hiding this comment.
Let's keep the scope to addressing the division by zero issue. Test looks good.
Please open a new issue to extend testing to np.array and pd.Series types.
There was a problem hiding this comment.
thanks @cwhanse
sure i will open issues separately for each np.array and pd.Series
There was a problem hiding this comment.
thanks @cwhanse sure i will open issues separately for each
np.arrayandpd.Series
Just one issue for both of them types is enough.
There was a problem hiding this comment.
done @echedey-ls
thanks for your reply!!
echedey-ls
left a comment
echedey-ls
left a comment
There was a problem hiding this comment.
LGTM. Add a whatsnew entry in the bugfixes section and this is good IMO.
@aman-coder03, just so you know, a cleaner and modular approach to write the tests would be to use @pytest.mark.parametrize, so you could avoid boiler plate repeated in the test at the top. Let me know if you wanna give it a try, it would definitely look better and introduce you to some of the perks of the testing framework.
tests/test_transformer.py
Outdated
|
|
||
|
|
||
| def test_simple_efficiency_zero_load_loss(): | ||
|
|
|
@echedey-ls i added a |
|
Feel free to merge this test at the top: https://github.com/aman-coder03/pvlib-python/blob/d45d6ea29165380d076481decc555af6b5a366bf/tests/test_transformer.py#L7C1-L41C1 |
cwhanse
left a comment
cwhanse
left a comment
There was a problem hiding this comment.
The change needs to be moved to a v0.13.3.rst. I've submitted a PR to create that file.
|
|
||
|
|
||
| Bug fixes | ||
| * Fixed a division-by-zero error in |
There was a problem hiding this comment.
| * Fixed a division-by-zero error in | |
| * Fix a division-by-zero condition in |
|
@echedey-ls I think it's OK to re-implement the tests is a separate PR, to avoid scope creep in a PR that addresses a division-by-zero condition. |
|
@cwhanse I agree with that. |
|
thanks both for the guidance @echedey-ls @cwhanse |
3 participants
fixes a
ZeroDivisionErrorinpvlib.transformer.simple_efficiencywhen
load_loss = 0.when
load_lossis zero, the transformer loss model reduces to a linearrelationship rather than a quadratic one. this PR handles that case
explicitly to avoid division by zero while preserving existing behavior
for
load_loss > 0.Fixes #2645