[DEMO: DO NOT MERGE] Brentq and Halley's method algorithm illustrations #412
Conversation
markcampanelli
changed the title
|
I'm not sure about all this commit history, because I tried to first sync up my fork with the upstream master. Ugh. I was getting a unit test failure on pvlib/test/test_pvsystem.py::test_ashraeiam (due to mismatch pandas and ndarray types), but then I also got it on master, so we'll see what CI does here. |
|
Turns out I was using some wonky pytest version and I needed to conda install the latest in my virtual env to get my local tests to run properly. |
pvlib/pvsystem.py
Outdated
| # Compute initially with zero r_s_Ohm | ||
| current_ic = photocurrent - saturation_current*np.expm1(voltage/nNsVth) - conductance_shunt*voltage | ||
|
|
||
| f = lambda current, conductance_shunt, resistance_series, nNsVth, voltage, saturation_current, photocurrent: current_sum_diode_node(voltage, current, conductance_shunt, resistance_series, nNsVth, saturation_current, photocurrent) |
There was a problem hiding this comment.
IMO if you should only use lambda for an anonymous inline function, if you are going to write it on it's own line, then you should write it as a function instead so either do this:
def f(lambda current, conductance_shunt, resistance_series, nNsVth, voltage, saturation_current, photocurrent):
return current_sum_diode_node(voltage, current, conductance_shunt, resistance_series, nNsVth, saturation_current, photocurrent)
or move this down to where you use f on line 2075.
see pep 8
There was a problem hiding this comment.
Also would be lovely to see these lines wrapped down to < 80 characters so I can read them without scrolling. thanks!
There was a problem hiding this comment.
This code is for illustration only, so please go easy on the over-conciseness with my lambda functions. I did add many comments :). That said, I'm sorry the formatting isn't better: I haven't figured out yet how to set up VSCode to auto format and for personal projects I just let my editor wrap my code to the window on my 14" laptop.
|
Looks like it is easier to hit edge cases when picking brentq bracketing intervals for the v_from_i() case, so I decided to illustrate my original Halley's method (i.e., scipy.optimize.newton()) implementation instead. To be clear: I would want to do a lot more testing (or perhaps numerical analysis) to show that either of these methods always converge (cf. #411). However, IMHO, the implementations are so simple (but perhaps slower due to vectorization wrappers) that I remain hopeful. |
markcampanelli
changed the title
|
So, now that I've added range checks for [0, Voc] and [0, Isc], I no longer get the failing unit test apparently due to bad bracketing on brentq(). That makes me feel more confident in this "fool proof" approach for positive power quadrant only. The downside is that very many of the @mikofski I have to admit that I'm not totally sure how the bishop-based solution offers much beyond this. |
|
Maybe best to first make and merge a new PR that puts the tests in the right quadrant. I made many of the numerical tests very quickly thinking that something was better than nothing to ensure stability. |
| raise ValueError("Current input out of range [0., Isc], current = {}, Isc = {}".format(current, i_sc(conductance_shunt, resistance_series, nNsVth, saturation_current, photocurrent))) | ||
|
|
||
| # Compute initially for ideal device with zero resistance_series and zero conductance_shunt | ||
| b = nNsVth*(np.log(photocurrent + saturation_current - current) - np.log(saturation_current)) |
There was a problem hiding this comment.
It's not clear how many iterations this might save, rather than just using Voc.
| raise ValueError("Voltage input out of range [0., Voc], voltage = {}, Voc = {}".format(voltage, v_oc(conductance_shunt, resistance_series, nNsVth, saturation_current, photocurrent))) | ||
|
|
||
| # Compute initially for ideal device with zero resistance_series and zero conductance_shunt | ||
| b = photocurrent - saturation_current*np.expm1(voltage/nNsVth) |
There was a problem hiding this comment.
It's not clear how many iterations this might save, rather than just using Isc.
markcampanelli
changed the title
|
@thunderfish24 , I'd really appreciate your review of scipy PR 8357 I think it does a good job at addressing your desire to vectorize these (newton, halley, secant) methods. The difference in speed is tremendous, using numpy outperforms cython by 10x, and numpy is a lot easier to use. You can post comments on scipy PR 8357, scipy issue 8354 or on the scipy-dev maillist. I'm concerned that if we don't push our agenda, then we'll be forced to use cython and that will increase the burden on us to maintain esoteric code. ditto for you @wholmgren , please comment on scipy PR8357. thanks! |
…into brentq_algorithms
|
This was for demo purposes only. Closing. |
This is a PR to illustrate usage of brentq() in place of LambertW for i_from_v() function.
v_from_i() should be very similar and I hope to add it soon.
pvlib python pull request guidelines
Thank you for your contribution to pvlib python!
You may submit a pull request with your code at any stage of completion, however, before the code can be merged the following items must be addressed:
git diff upstream/master -u -- "*.py" | flake8 --diffand/or landscape.io linting service.docs/sphinx/source/api.rstfor API changes.docs/sphinx/source/whatsnewfile for all changes.Please don't hesitate to ask for help if you're unsure of how to accomplish any of the above. You may delete all of these instructions except for the list above.
Brief description of the problem and proposed solution (if not already fully described in the issue linked to above):