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

Add minus sign in front of dEgdT, sdm.py #2322

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

echedey-ls
Copy link
Contributor

@echedey-ls echedey-ls commented Dec 3, 2024

Fixes the typo in line 874 of sdm.py, where dEgdT = -0.0002677 instead of the same without the minus sign.

Inspired by today's AdventOfCode, I've regexed if this typo is everywhere else and I can confirm it isn't. Regex: (?<!-)0.0002677

Tests are passing locally. @cwhanse , should I create a test with a hella lot of precision to test this?

Co-Authored-By: Cliff Hansen <[email protected]>
Co-Authored-By: Kevin Anderson <[email protected]>
@echedey-ls echedey-ls self-assigned this Dec 3, 2024
@echedey-ls echedey-ls added the bug label Dec 3, 2024
@echedey-ls echedey-ls added this to the v0.11.2 milestone Dec 3, 2024
@cwhanse
Copy link
Member

cwhanse commented Dec 3, 2024

should I create a test with a hella lot of precision to test this?

I'd say no, it confirms the error is inconsequential, for practical purposes.

@RDaxini
Copy link
Contributor

RDaxini commented Dec 4, 2024

it confirms the error is inconsequential, for practical purposes.

This surprises me...
I haven't looked in detail yet at how the tests are constructed, but, a simple example:

import pandas as pd
from pvlib.pvsystem import calcparams_desoto, singlediode

ee = pd.Series([500, 800, 1000])
tc = pd.Series([40, 50, 60])
alphasc = 0.0004
aref = 1
ilref = 5
ioref = 1e-10
rshref = 2e3
rs = 1
egref = 1.1
degdt_pos = 0.0002677
degdt_neg = -0.0002677

sde_param1 = calcparams_desoto(ee, tc, alphasc, aref, ilref, ioref, rshref, rs,
                               egref, degdt_pos)
sde_param2 = calcparams_desoto(ee, tc, alphasc, aref, ilref, ioref, rshref, rs,
                               egref, degdt_neg)

output1 = singlediode(sde_param1[0], sde_param1[1], sde_param1[2],
                      sde_param1[3], sde_param1[4])
output2 = singlediode(sde_param2[0], sde_param2[1], sde_param2[2],
                      sde_param2[3], sde_param2[4])

returns different values between output1 and output2. Have I done something incorrectly here?

@cwhanse
Copy link
Member

cwhanse commented Dec 4, 2024

The error affects ivtools.sdm.fit_desoto_sandia, not pvsystem.calcparams_desoto and singlediode.

The tests for fit_desoto_sandia have a coarse tolerance, which could have been set because of this error.

assert np.allclose(modeled[params.keys()].values,

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.

pvlib.ivtools.sdm.fit_desoto_sandia has incorrect sign for returned dEgdT
3 participants