Skip to content

Added rounding of aa and b in _schumaker_qspline.py, see Issue #1311 #1315

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

Merged
merged 4 commits into from
Nov 3, 2021

Conversation

Antoine-0
Copy link
Contributor

@Antoine-0 Antoine-0 commented Sep 27, 2021

  • Closes Spline fitting failure because of rounding #1311
  • I have read the contributing guidelines
    I'm not sure how to do the following items :
  • Tests added
  • [ ] Updates entries to docs/sphinx/source/api.rst for API changes.
  • Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/whatsnew for all changes. Includes link to the GitHub Issue with :issue:`num` or this Pull Request with :pull:`num`. Includes contributor name and/or GitHub username (link with :ghuser:`user`).
  • Pull request is nearly complete and ready for detailed review.
  • Maintainer: Appropriate GitHub Labels and Milestone are assigned to the Pull Request and linked Issue.

@cwhanse
Copy link
Member

cwhanse commented Sep 27, 2021

I'll take a look at the failing test. I can add the text for the whatsnew document, and take care of assigning labels. Don't worry about the API item.

@cwhanse
Copy link
Member

cwhanse commented Oct 7, 2021

I removed the data that was causing the test failure. Those data were made up to check consistency between Matlab and python implementations of pvlib.ivtools.sde._fit_sandia_cocontent. The test still has 2 of the original 3 made-up IV curves, which pass the test criteria. So I am not concerned about dropping part of the test data, the test still meets its intent. The removed data that are removed would still produce values for the 5 IV curve parameters, but they would not be close to what is produced by the Matlab code (e.g. 16% relative difference in one parameter).

I also split the EPS parameter into EPS_slope to determine when a slope is 0, and EPS_val to determine when a number is zero. The EPS_slope value is around 10^-6, the EPS_val value is single digit precision.

Why did the removed data fail the test? The single-curve fitting function is rather sensitive to its input data. Rounding small values to 0 changes the spline fit, which affects the cocontent integral, which affects a regression, etc. This sensitivity is why the diode model fitting in pvlib.ivtools.sdm.fit_pvsyst_sandia (and similar) retains only the shunt resistance from the single IV curve fits; the other four parameters aren't reliably estimated by the cocontent method.

@cwhanse cwhanse added the bug label Oct 7, 2021
@cwhanse cwhanse added this to the 0.9.1 milestone Oct 7, 2021
@cwhanse
Copy link
Member

cwhanse commented Oct 17, 2021

any objections to merging this?

@Antoine-0
Copy link
Contributor Author

no, this is all fine for me, thanks again for running the tests !

Copy link
Member

@wholmgren wholmgren left a comment

Choose a reason for hiding this comment

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

Without carefully engaging with the issue and only looking at the diff, I have two quick comments:

  1. We need a 0.9.1 entry in the whatsnew.rst file here: https://github.com/pvlib/pvlib-python/blame/master/docs/sphinx/source/whatsnew.rst#L8 Edit - fine to do this in a follow up PR.
  2. Would it be better to add a new test that asserts the correct behavior rather just remove a test? Edit - I read Cliff's comment above and it seems fine just drop the test. Sorry for writing too quickly.

@cwhanse
Copy link
Member

cwhanse commented Nov 3, 2021

will merge when CI is done

@cwhanse cwhanse merged commit 61e9582 into pvlib:master Nov 3, 2021
@cwhanse
Copy link
Member

cwhanse commented Nov 3, 2021

Thanks @Antoine-0

@Antoine-0
Copy link
Contributor Author

I merely noticed an issue, thanks to you for updating the tests and the great work you put into pvlib !

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.

Spline fitting failure because of rounding
4 participants