Conversation
|
Once this is accepted, next PR will replace calls to scipy's |
|
Put this in pvlib docs don't provide links to public function in either of these folders so there's no API change. |
echedey-ls
left a comment
echedey-ls
left a comment
There was a problem hiding this comment.
In general terms, I'd prefer to keep this function internal. Or at least give it some internal period just in case anything arises later. A function in the API is somewhat a contract with the user, and even thou I'm confident that it works pretty well, I'm not sure about the maintenance purely numerical API may bring later, or if users start assuming it follows numpy/scipy conventions... On the other hand, the more people that use it, the better to confirm that it works. I don't have a strong opinion.
Last big comment: what is the source of the equations? scipy's implementation? any source linked by them? I would add it for the sake of completeness. Sry if I missed it previously.
I don't mind its placement; thou I prefer pvlib.ivtools.utils, the nearest junk drawer to where it will be used.
Some minor comments below.
tests/test_tools.py
Outdated
| def test_lambertw_pvlib(): | ||
| test_exp = np.arange(-10., 300, step=10) | ||
| test_x = 10.**test_exp | ||
| # known solution from scipy.special.lambertw |
There was a problem hiding this comment.
Can you please add the exact version of scipy and date you generated it? For traceability. Else, we could generate the comparison on the fly, so if there ever is a change at scipy we can catch it soon.
Co-authored-by: Echedey Luis <80125792+echedey-ls@users.noreply.github.com>
It's direct application of two well-known methods (Halley's, Newton) to the Lambert W function. I can write a derivation (a few lines) in the function documentation, if that helps.
I agree, moved to ivtools.utils |
I retract my opinion, it should have been clear to me when I read the code. |
There was a problem hiding this comment.
Seems like convergence is poor as it approaches the branch point at -1/e. Should we tighten that up and test for negative inputs?
I also see larger error for 10 < x < 100 or so, although "larger" is still quite small. Still, it seems like it could be easily eliminated by switching methods at x == 100 instead of x == 10?
pvlib/ivtools/utils.py
Outdated
| x : np.array | ||
| Must be real numbers. |
There was a problem hiding this comment.
Will requiring np.array (i.e., not allowing float) be a problem for supporting single-value I-V curve calculations?
There was a problem hiding this comment.
I think it should accept float, added that
Co-authored-by: Kevin Anderson <kevin.anderso@gmail.com>
The problem at the branch point is that the derivative -> infinity as x -> -1/e. Accurate estimation for negative x approaching -1/e will require a different technique (note scipy has a bug at the branch point scipy/scipy#24770). In the solution for the single diode equation, the argument of
Done. 10 was just my guess, I didn't draw the error figure like yours, that's great. Also, I added |
Co-authored-by: Echedey Luis <80125792+echedey-ls@users.noreply.github.com>
3 participants
docs/sphinx/source/referencefor API changes.docs/sphinx/source/whatsnewfor 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`).remote-data) and Milestone are assigned to the Pull Request and linked Issue.Add a real-valued-only Lamberts W function. As tested, function is as precise as scipy's lambertw, and 4x faster due to not doing all arithmetic with complex values.