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 "logwright" SDE solver functions #1884

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

kandersolar
Copy link
Member

  • Closes Add an SDE solver using the "LogWright" function  #1856
  • I am familiar with the contributing guidelines
  • Tests added
  • Updates entries in docs/sphinx/source/reference 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`).
  • New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
  • Pull request is nearly complete and ready for detailed review.
  • Maintainer: Appropriate GitHub Labels (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

This PR implements two new functions in pvlib.singlediode: _logwright_v_from_i and _logwright_i_from_v. The reference from #1856 (link) provides the v_from_i equation. I used the same technique to derive the i_from_v equation. I plan to document the equations in our singlediode.rst page.

The new base functions are then integrated into the rest of the SDE solving machinery. Here are some runtimes for the pvsystem.i_from_v, v_from_i, and singlediode functions comparing method='lambertw' with method='logwright':

image

@kandersolar kandersolar added this to the v0.10.3 milestone Oct 6, 2023
@kandersolar
Copy link
Member Author

@cwhanse I wonder if you can provide some insight on the calculation result differences I am seeing, relative to the values from method='lambertw'. In particular the identified vmp and its downstream values are much less accurate than the other values, so I suspected something with the golden section search may be the cause. However, setting a stricter tolerance in the golden section search doesn't make much difference. I also looked at the differences from logwright to newton and lambertw to newton, and they're all about the same as this:

image

@cwhanse
Copy link
Member

cwhanse commented Oct 6, 2023

@kandersolar you are plotting Vmp (logWright) - Vmp (lambertw)?

What happens if you increase the iterations in tools._logwrightomega?

@cwhanse
Copy link
Member

cwhanse commented Oct 6, 2023

The curves in the precise_iv_curve*.json files are precise (to 15 significant figures), maybe we can benchmark the three solvers against those (lambertw, log Wright, netwon). Sorry if the format is inconvenient.

@kandersolar
Copy link
Member Author

@kandersolar you are plotting Vmp (logWright) - Vmp (lambertw)?

Yep.

What happens if you increase the iterations in tools._logwrightomega?

No meaningful change. I also tried switching to scipy's implementation of the Wright Omega function, and no change there either.

The curves in the precise_iv_curve*.json files are precise (to 15 significant figures), maybe we can benchmark the three solvers against those (lambertw, log Wright, netwon).

Good idea! I will try that.

@kandersolar
Copy link
Member Author

Here is a comparison of the various methods, calculating error relative to the "Case 1" known values in https://github.com/cwhanse/ivcurves/tree/main/ivcurves/test_sets:

image

So newton and brentq do quite well, but the two lambertw-style approaches have some trouble finding the MPP.

Putting aside logwright for the moment, I think I have narrowed the MPP issue down to the golden section search function performing poorly at the peak of the P-V curve, which is relatively flat and broad. Specifically, P varies by only an ULP or two (~10^-15) over a voltage range of order 10^-7, so unsurprisingly the golden section search has trouble navigating that region. Here is a plot showing the P-V curve and comparing identified MPPs (horizontal lines show a 1-ULP step, since matplotlib refuses to show tick labels at such a small scale):

image

Just to see if another search method might work better, I swapped scipy's powell in place of our golden section search and got somewhat better convergence in this single example, but still not as good as the bishop/brentq method. I am curious how it is that brentq seems to handle this problem so reliably.

Anyway, this is starting to sound like a bug report for _golden_sect_DataFrame. Perhaps this should be a new issue?

@cwhanse
Copy link
Member

cwhanse commented Oct 10, 2023

Nicely laid out. Your findings are consistent with my own assessments.

The lambert W methods find the maximum of the (V, P) curve. The bishop88 methods find the zero of the dP/dV curve. I'm not sure the difference is a bug in the usual sense. When that approach was taken, I'm sure we thought 8 digits of accuracy sufficed, and may not have realized the shortcoming in that approach.

@adriesse
Copy link
Member

Interesting observations and discussions! If the blue line above from the precise IV curve, would it be possible to make it even more precise, or perhaps more ideal, by removing the dithering so it steps up on the left and down on the right of Pmp?

@adriesse
Copy link
Member

I seem to recall another discussion about (vectorized?) root finding recently, but I can't remember where. Could someone point me to it?

@cwhanse
Copy link
Member

cwhanse commented Oct 10, 2023

#1661

@kandersolar
Copy link
Member Author

Or #974.

If the blue line above from the precise IV curve, would it be possible to make it even more precise, or perhaps more ideal, by removing the dithering so it steps up on the left and down on the right of Pmp?

Shame on me for not labeling it! The blue line in that plot was from lambertw_i_from_v. The precise curves would presumably not suffer from such numerical artifacts, but they also doesn't have nearly the voltage resolution needed to show them anyway.

@adriesse
Copy link
Member

Or #974.

That's the one, thanks!

@mikofski
Copy link
Member

there was a similar issue in pvmismatch which was using argmax to find the max power point (MPP), but in issue 65 there were discontinuities/jumps similar to the issue described for LambertW approach. A simple solution was to calculate the derivative around MPP using central difference and interpolate to find a better zero.

@adriesse
Copy link
Member

If the blue line above from the precise IV curve, would it be possible to make it even more precise, or perhaps more ideal, by removing the dithering so it steps up on the left and down on the right of Pmp?

Shame on me for not labeling it! The blue line in that plot was from lambertw_i_from_v. The precise curves would presumably not suffer from such numerical artifacts, but they also doesn't have nearly the voltage resolution needed to show them anyway.

Does the dithering also appear in V and I, or only their product? If only the latter, perhaps there is a way to make a cleaner product?

@cwhanse cwhanse mentioned this pull request Dec 8, 2023
15 tasks
@kandersolar kandersolar modified the milestones: v0.10.3, v0.10.4 Dec 20, 2023
@kandersolar kandersolar modified the milestones: v0.10.4, Someday Feb 23, 2024
@echedey-ls
Copy link
Contributor

Mmmm, can I be of help with this PR? I see the discussion revolves around the precision of each algorithm, but that doesn't invalidate the fact that logwright is still faster (if I understood correctly).

@kandersolar
Copy link
Member Author

The convergence error discussed above is large enough to make me uncomfortable, but probably not too large to keep the method from being useful. The other thing is that only the v_from_i equation has a reference; I derived the i_from_v equation myself.

This PR currently shows that the logwright method has promise, but IMHO a short paper defining the complete method and comparing speed+accuracy with other methods needs to be written before this PR gets picked up again.

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.

Add an SDE solver using the "LogWright" function
5 participants