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

Parameters missing from ivtools.sdm.fit_desoto_sandia returns statement #2315

Closed
RDaxini opened this issue Dec 3, 2024 · 3 comments · Fixed by #2317
Closed

Parameters missing from ivtools.sdm.fit_desoto_sandia returns statement #2315

RDaxini opened this issue Dec 3, 2024 · 3 comments · Fixed by #2317
Milestone

Comments

@RDaxini
Copy link
Contributor

RDaxini commented Dec 3, 2024

Is your feature request related to a problem? Please describe.
In addition to the parameters already listed in the returns statement in the docs of pvlib.ivtools.sdm.fit_desoto_sandia, this function also returns a_ref and dEgdT, which are two parameters required for the De Soto module performance model.

Describe the solution you'd like
Add descriptions of a_ref and dEgdT to the returns statement in the docs

Describe alternatives you've considered
Have I missed something here...?

@kandersolar
Copy link
Member

kandersolar commented Dec 3, 2024

@cwhanse I notice a possible sign error: fit_desoto_sandia returns dEgdT=0.0002677 while calcparams_desoto defaults to dEgdT=-0.0002677. Is this a bug?

If so, it may affect the matlab version as well: https://github.com/sandialabs/MATLAB_PV_LIB/blob/master/pvl_desoto_parameter_estimation.m#L276

@cwhanse
Copy link
Member

cwhanse commented Dec 3, 2024

@cwhanse I notice a possible sign error: fit_desoto_sandia returns dEgdT=0.0002677 while calcparams_desoto defaults to dEgdT=-0.0002677. Is this a bug?

Yes it is. New issue or fix here?

If so, it may affect the matlab version as well: https://github.com/sandialabs/MATLAB_PV_LIB/blob/master/pvl_desoto_parameter_estimation.m#L276

Technically, no, since the Matlab function doesn't return dEgdT.

@kandersolar
Copy link
Member

Opened #2321 so we can close this one with #2317.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants