8000 Add minus sign in front of dEgdT, `sdm.py` by echedey-ls · Pull Request #2322 · pvlib/pvlib-python · GitHub
[go: up one dir, main page]
Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Dec 13, 2024
Merged

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

merged 8 commits into from
Dec 13, 2024

Conversation

echedey-ls
Copy link
Contributor
@echedey-ls echedey-ls commented Dec 3, 2024 •
edited
Loading

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?

@echedey-ls @cwhanse @kandersolar
Add - in front of dEgdT
535e3dc 
Co-Authored-By: Cliff Hansen <5393711+cwhanse@users.noreply.github.com>
Co-Authored-By: Kevin Anderson <57452607+kandersolar@users.noreply.github.com>
@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 •
edited
Loading

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_par
8000
am2 = 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 •
edited
Loading

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.

pvlib-python/pvlib/tests/ivtools/test_sdm.py

Line 141 in 6af80da

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

@echedey-ls
Copy link
Contributor Author

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

At first I thought similar, but I decided to test it and it does not seem to change anything noticeable.

My test strategy has been:

  1. Switch between +/- dEgdT
  2. Run the tests without the tolerance, which made them fail
  3. Re-run the tests and breakpoint on that assert, run command modeled[params.keys()].values-expected[params.keys()].values.

There is no difference, I've added my results to the details section down below.

Now I wonder whether that term has any impact on the model (I suppose it does, nobody would have put it there without a reason). Is there any check I've missed? Maybe is the max cell temp difference from STC too low (20 ºC)?

Same output 

fit_desoto_sandia
-----------------
+ 0.0002677
Mismatched elements: 2 / 5 (40%)
Max absolute difference among violations: 10.26629715
Max relative difference among violations: 0.07620939

modeled[params.keys()].values-expected[params.keys()].values =
array([-7.84332362e-05,  7.69714883e-12,  1.02662972e+01, -4.47022678e-03,
        7.18454498e-03])

- 0.0002677
Mismatched elements: 2 / 5 (40%)
Max absolute difference among violations: 10.26629715
Max relative difference among violations: 0.07620939

modeled[params.keys()].values-expected[params.keys()].values =
array([-7.84332362e-05,  7.69714883e-12,  1.02662972e+01, -4.47022678e-03,
        7.18454498e-03])

@cwhanse
Copy link
Member
cwhanse commented Dec 8, 2024 •
edited
Loading

The way that algorithm works, the value of dEgdT only affects EgRef and I_o_ref, and the effect on I_o_ref is very small. Here's a script to illustrate. sdm_desoto_degdt is a copy of pvlib\ivtools\sdm.py with lines 857-858 changed:

    elif model == 'desoto':
        dEgdT = 0.0002677 * degdt_sign

Pass degdt_sign into _extract_sdm_params from fit_desoto_sandia. Give it values of either +1 or -1.

import numpy as np
import sdm_desoto_degdt as sdd
from pvlib import pvsystem


cansol = {'ivcurve': {'V_mp_ref': 46.6, 'I_mp_ref': 4.73, 'V_oc_ref': 58.3,
                        'I_sc_ref': 5.05},
            'specs': {'alpha_sc': 0.0025, 'beta_voc': -0.19659,
                      'gamma_pmp': -0.43, 'cells_in_series': 96},
            'params': {'I_L_ref': 5.056, 'I_o_ref': 1.01e-10,
                       'R_sh_ref': 837.51, 'R_s': 1.004, 'a_ref': 2.3674,
                       'Adjust': 2.3}}
params = cansol['params']
params.pop('Adjust')
specs = cansol['specs']
effective_irradiance = np.array([400., 500., 600., 700., 800., 900.,
                                 1000.])
temp_cell = np.array([15., 25., 35., 45.])
ee = np.tile(effective_irradiance, len(temp_cell))
tc = np.repeat(temp_cell, len(effective_irradiance))
IL, I0, Rs, Rsh, nNsVth = pvsystem.calcparams_desoto(
    ee, tc, alpha_sc=specs['alpha_sc'], **params)
ivcurve_params = dict(photocurrent=IL, saturation_current=I0,
                      resistance_series=Rs, resistance_shunt=Rsh,
                      nNsVth=nNsVth)
sim_ivcurves = pvsystem.singlediode(**ivcurve_params).to_dict('series')
v = np.linspace(0., sim_ivcurves['v_oc'], 300)
i = pvsystem.i_from_v(voltage=v, **ivcurve_params)
sim_ivcurves.update(v=v.T, i=i.T, ee=ee, tc=tc)


result = sdd.fit_desoto_sandia(sim_ivcurves, specs)
for k in ['I_L_ref', 'I_o_ref', 'R_s', 'R_sh_ref', 'a_ref', 'EgRef', 'dEgdT']:
    print(f'{k}: {result[k]}')

result2 = sdd.fit_desoto_sandia(sim_ivcurves, specs, degdt_sign=-1)
for k in ['I_L_ref', 'I_o_ref', 'R_s', 'R_sh_ref', 'a_ref', 'EgRef', 'dEgdT']:
    print(f'{k}: {result2[k]}')

I_L_ref: 5.055921566763837
I_o_ref: 1.0869714883112935e-10
R_s: 0.999529773217279
R_sh_ref: 847.7762971522803
a_ref: 2.374584544977335
EgRef: 1.1174113417791087
dEgdT: 0.0002677

I_L_ref: 5.055921566763837
I_o_ref: 1.0869714883112857e-10
R_s: 0.999529773217279
R_sh_ref: 847.7762971522803
a_ref: 2.374584544977335
EgRef: 1.3112547292120789
dEgdT: -0.0002677

@kandersolar
Copy link
Member

So the problem is not that the change makes no difference, the problem is that our tests don't check the affected quantities. The test currently only looks at ['I_L_ref', 'I_o_ref', 'R_sh_ref', 'R_s', 'a_ref']. Seems like we should add at least dEgdT and EgRef to the tested values in order to close out #2321.

There are other untested values as well ('iph', 'io', 'rsh', 'rs', 'u', 'cells_in_series'). Seems like we should test those as well, but that's not required for this PR IMHO.

@echedey-ls
Copy link
Contributor Author

These things have caught my attention:

  1. The tested module is made of poly-crystalline (from datasheet), for which in pvlib.pvsystem.calcparams_pvsyst states in the docstring a EgRef = 1.121 eV for crystalline silicon. The result here is 1.3112547292120638
  2. Values of results, lets say result["rs"] differ from Rs by an amount I'm unable to distinguish acceptable or not. Trace from an assert_allclose: 
    Mismatched elements: 28 / 28 (100%)
Max absolute difference among violations: 0.00804031
Max relative difference among violations: 0.00800828
    Before making too coarse tests I'd like to have your input.

@cwhanse
Copy link
Member
cwhanse commented Dec 12, 2024

These things have caught my attention:

  1. The tested module is made of poly-crystalline (from datasheet), for which in pvlib.pvsystem.calcparams_pvsyst states in the docstring a EgRef = 1.121 eV for crystalline silicon. The result here is 1.3112547292120638

The algorithm (as documented in the reference technical report) regards EgRef as a fitting parameter. I recall that fitting EgRef significantly improved the fitted model. Maybe worth a note in fit_desoto_sandia docstring.

  1. Values of results, lets say result["rs"] differ from Rs by an amount I'm unable to distinguish acceptable or not. Trace from an assert_allclose: 
    Mismatched elements: 28 / 28 (100%)
Max absolute difference among violations: 0.00804031
Max relative difference among violations: 0.00800828

That doesn't surprise me. The five returned vectors are the diode equation parameters one for each input IV curve. The five values are determined by fitting. The thought was that fitting each curve would reveal the structure of the relationship between each parameters and irradiance and temperature. The DeSoto model (and other single diode models) assume that structure is described by the model's auxiliary equations, in the case of rs, that Rs is a constant for all curves. That's why rs differs from Rs.

If we test the diode equation parameters rs etc. we ought to compare with archived values rather than diode model parameters e.g. Rs. IMO, testing at that intermediate level would help with diagnosis if the current test_fit_desoto_sandia fails, but if the current test passes (diode model parameters are preserved), then I think it is safe to assume the intermediate variables are also OK.

@echedey-ls
Copy link
Contributor Author

This PR should be ready to go. I don't have more time today so feel free to push changes as you need.

Thanks @cwhanse for the in-depth explanation, I wasn't aware of all that :)

@kandersolar kandersolar merged commit b50b56f into pvlib:main Dec 13, 2024
26 checks passed
@kandersolar
Copy link
Member

Thanks @echedey-ls!

@echedey-ls echedey-ls deleted the dEgdT-is-negative-in-fit_desoto_sandia branch December 13, 2024 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cwhanse cwhanse cwhanse approved these changes

Assignees

@echedey-ls echedey-ls

Labels
bug
Projects
None yet
Milestone
v0.11.2
Development

Successfully merging this pull request may close these issues.

pvlib.ivtools.sdm.fit_desoto_sandia has incorrect sign for returned dEgdT
4 participants
@echedey-ls @cwhanse @RDaxini @kandersolar
0