Skip to content

Conversation

@reepoi
Copy link
Contributor

@reepoi reepoi commented May 18, 2023

The ivcurve_pnts parameter of pvsystem.singlediode is deprecated in favor of using pvsystem.i_from_v and v_from_i to get additional points on an IV curve. The test cases and docs/examples/iv-modeling/plot_singlediode.py are updated. Also, a bug in tools._golden_sect_DataFrame where scalars were converted into 0d-arrays is fixed.

pvsystem.singlediode returns a dict or a pd.DataFrame. A dict is returned only when all inputs are scalars or ivcurve_pnts > 0.

@kandersolar
Copy link
Member

I think ivcurve_pnts deserves a deprecation period before being removed. @reepoi would you be up for modifying this PR to keep the parameter for now, but emit a deprecation warning if it gets used?

@reepoi reepoi force-pushed the singlediode-ivcurvepnts branch from 161522c to 4a1dfab Compare May 23, 2023 17:19
@reepoi reepoi changed the title Remove ivcurve_pnts from pvsystem.singlediode Deprecate pvsystem.singlediode parameter ivcurve_pnts May 23, 2023
@cwhanse cwhanse added this to the 0.10.0 milestone May 31, 2023
Copy link
Member

@cwhanse cwhanse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI failure is codecov, ignorable.

Copy link
Member

@kandersolar kandersolar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late review, I missed that this was tagged 0.10.0! Looks good to me overall; the only thing I see is that these deprecation warnings from the tests need to get cleaned up: https://github.com/pvlib/pvlib-python/actions/runs/5201899928/jobs/9382722429?pr=1743#step:9:69

@reepoi
Copy link
Contributor Author

reepoi commented Jun 13, 2023

Ok, I wrapped all the tests using pvsystem.singlediode's ivcurve_pnts parameter with with pytest.warns to hide the warning showing in the pytest output.

Copy link
Member

@kandersolar kandersolar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks @reepoi!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants