fix bad atol in singlediode test #415
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
looks good to me. I had a good laugh, because I'm using the new branch, and forgot to set In [2]: singlediode(7, 6e-7, .1, 20, .5)
Out[2]:
OrderedDict([('i_sc', 6.9651723221583133),
('v_oc', 8.1063001465863103),
('i_mp', 6.1362673597376753),
('v_mp', 6.2243393757884284),
('p_mp', 38.194210547580511),
('i_x', 6.7558815684409561),
('i_xx', 4.2640604802975011)])I thought, I'm going crazy. But then I remembered. I get the same as you. The tolerance looks good. In [3]: singlediode(7, 6e-7, .1, 20, .5, method='lambertw')
Out[3]:
OrderedDict([('i_sc', 6.965172322158323),
('v_oc', 8.106300146586307),
('i_mp', 6.127538662298922),
('v_mp', 6.2331331636234655),
('p_mp', 38.19376444736038),
('i_x', 6.755881568440956),
('i_xx', 4.249847011154563)])thanks! |
mikofski
added a commit
to mikofski/pvlib-python
that referenced
this pull request
Jun 20, 2018
* use `method='lambertw'` for singlediode tests * update arguments for calcparams_desoto everywhere with effective_irradiance and expand model parameters a_ref, I_L_ref, I_o_ref, R_sh_ref, and R_s
wholmgren
pushed a commit
that referenced
this pull request
Jul 10, 2018
…f single-diode model (#409) * add methods and tests for a faster way to calculate single-diode model * log estimate of speedup in test * use ordered dict for output, match inputs and outputs, add iv-curve * add "slow" but reliable ways to calculate i_from_v and vv * add slow_i_from_v(v, *x) which uses bishop88 and the canned bisecton method from scipy.optimize.fminbound to guarantee convergence * ditto for slow_v_from_i(i, *x) * ditto for slow_mppt(*x) * add slower_way(*x) which is the equivalent of faster_way(*x) but using the canned bisection method from scipy.optimize.fminbound instead of the generic newtom step method. * also add some better docstrings and comments Signed-off-by: Mark Mikofski <[email protected]> * add check for numerical errors * use float64, output symbols for fun, make executable * add tests for slower_way using fminbound test_way_faster --------------- * change test names to test_fast_ and test_slow to differential * move tstop before the expansion of output values way_faster ---------- * refactor to use photocurrent for il, saturation_current for io, resistance_series for rs, resistance_shunt for rsh, and nNsVth for nnsvt * remove start of custom newton solver * fix FIXME to use the correct names * change voc to v_oc * add numeric type to args in docstrings * so that pycharm will stop complaining * add a bunch of todo's for #408 and #410 * use DELTA instead of recalculating each time * fix typo/bug in docstring for nNsVth arg, missing trailing colon (:) had quotes (") instead * replace custom newton method with scipy.optimize.newton * remove tol, damp, log, & test args in from faster_way() since not used anymore * do not import logging, and remove LOGGER, etc. * also remove EPS, DAMP, DELTA all not used * import newton * add fast_i_from_v() using newton * remove the entire custom newton code in faster_way for finding the real voc and replace it with a call to newton, passing func and fprime, and return the value of v from bishop88() using optimum vd * ditto for isc and mpp * get lambdas working in fast_i_from_v * add fast_v_from_i using newton * add fast_mppt * calculate i_x and i_xx too for both fast and slow * use fast_i_from_v, fast_v_from_i, and fast_mppt in faster_way * change names in faster_way to match the same notation eg: v_oc instead of voc_est * test i_x and i_xx too * since v_mp doesn't match because of different tolerances, use pvsystem.i_from_v(v=(voc+vmp)/2) to get ixx for test * don't need to save isc as temp variable, only voc, so calculate it on assignment into "out" the ordered dict * use brentq instead of fminbound * add gradients bool arg to bishop88(), only outputs gradients if true, otherwise just i, v, and p * output di/dv too, why not? * brentq finds zeros, not minimums, like newton, so don't need to square residual in any slow_ * change lambda to pass args from brentq * grad_v is now index 4 (5th item) * grad_i is now index 3 (4th item) * grad_p is now index 6 (7th item) * grad2p is now index 7 (8th item) * since brentq finds zero, not minimum, don't use power in slow_mppt, use grad_p * change test for numerical errors to use data file * make old test generate data, add to data folder * make new test read data * try to figure out clever way to not need sympy * working on #410 * add method arg and make slow the default * check size of photocurrent and vectorize method if necessary * reorganize out if len(photocurrent) * helping test_singlediode pass tests * in any test where mpp is tested, test lambertw first separately, then use i_from_v to calculate the corresponding value and store it in the expected values * * add the requires scipy decorator * still 2 tests in model chain strugling * fixes #410 * test_modelchain now passes * in pvsystem.singlediode if using default "slow" then use ducktyping to see if photocurrent is a sequence, sorry other params are not checked, if not, then get out as usual, if it is, then use np.vectorize, and get a sequence of outputs, check if photocurrent is a pd.Series, and if so, then convert out to a pd.DataFrame, otherwise, transpose the sequence of outputs into a an OrderedDict of sequences (actually np.arrays) * move series check from original lambertw back up to there, because it doesn't work for the vectorized slow or fast methods, you need to call .tolist() first * also copy & paste WET code for 'fast' * add i, v, and p to out * also use numpy arrays not pd.Series in OrderedDict out by using pd.Series.value * also use np.vstack to reshape array of arrays inside i, v, and p * need to calculate i-v curve points to compare * using pvsystem.i_from_v and v_from_i, use transpose twice to get it in the right shape * also only output i, v, and p when there actually are ivcurve_pnts * add mppt method, update docs * add mppt method that returns an OrderedDict with `i_mp`, `v_mp`, and `p_mp` or a pandas series with the same * add `pvlib.pvsystem.mppt` to api.rst * add section to single diode that explains the different methods, proves that the problem is bounded, and that is guaranteed to converge * add reference to bishop * combine redundant code and remove FIXME * add notes to what's new * duck type scipy.optimize imports in way_faster.py and remove TODO's Signed-off-by: Mark Mikofski <[email protected]> * fix latex and other sphinx formatting issue sand typos * fix circular import issues? * add bishop88 and est_voc to pvsystem api * add 0.5.2 to what's new * literally add bishop88 and est_voc to pvsystem from way_faster * fix some typos * explicitly state that these point bound the entire forward bias 1st quadrant iv curve * remove, "we'll remove fast in the future", why? * fix link to bishop88 * fix links in est_voc and add equation and reference * add reference in bishop88 * do not import way_faster ANYWHERE! - in test_numerical_precision.py use pvsystem.est_voc and pvsystem.bishop88 - in test_way_faster.py define faster_way = pvsystem.way_faster.faster_way and ditto for slower_way Signed-off-by: Mark Mikofski <[email protected]> * try something remove redundant package imports * don't raise import error at module level if no scipy * okay I think I fixed it, now return pvlib module objects * try to modify i_from_v * implement v_from_i wrapper * add new test_i_from_v_from_i which recycles the test fixture from v_from_i which has more reasonable test values that are not way down in quadrant 4, it uses the old i_from_v(method='lambertw') to recalculate I so it has the write shape, size and dtype * set method='lambertw' for old i_from_v test, because since the Voc is like 8V, and the test voltage is 40V, the expected current is -300A, which is unlikely, and I doubt very very very much that this calculated value is anywhere near the actual current. Ha, ha, ha, 300A, that is so funny! * change PVSystem_i_from_v test to use the v_from_i fixture values which actually make some sense * set v_from_i inside singlediode, to use the method='lambertw' when given * add default method='' to v_from_i, add docstring * add docstring to i_from_v * add section to size and shape * wrap i_from_v_fun with returns_nan(), defaults to ValueError and RuntimeError * add returns_nan() decorator to way_faster * respond to @thunderfish24 review items: * fix spelling in what's new decent -> descent * refactor/rename way_faster.py -> singlediode_methods.py, thanks pycharm * refactor/rename test_way_faster.py -> test_singlediode_methods.py * * change default argument for method to "gold" * it doesn't matter what this is called * also fix spelling precicion -> precision * update test_singlediode_methods to use lambertw in comparison * add comment about how Voc is estimated and that it's useful as a bound for the bisection method * remove "unreliable" from fast/newton methods * change name from mppt -> mpp * add mpp tests for float, array and series * * add test to check for verbose yet graceful failure of v_from_i * check size of all args * return value error if this_size > size and size > 1 * fix pytest.raise context, add test_v_from_i * add all i_from_v and v_from_i args to checklist * remove enumerate, not used * add some comments to explain why we do it * combine assignment of size, shape initial values in one line * use *args in calls to i_from_v, etc. since we have it * don't need to check if size<=1 before updating size or shape, since we let np.vectorize handle broadcasting and raising ValueError * change mppt->mpp in api.rst docs * fix what's new to point to mpp in docs, also add links to bishop88 and voc_est * TST: fix precision test to use new calcparams* API * TST: update singlediode test for updated atol in #415 * use `method='lambertw'` for singlediode tests * update arguments for calcparams_desoto everywhere with effective_irradiance and expand model parameters a_ref, I_L_ref, I_o_ref, R_sh_ref, and R_s * DOC: update what's new for 0.6 with proposed explicit SDM solution * DOC: update docstring to conform to numpydoc style in sdm methods * ENH: refactor est_voc -> estimate_voc * in what's new "Implement :func:`pvlib.pvsystem.estimate_voc` ..." * in api.rst * as an object from singlediode_methods in pvsystem, hmmm, probably need to refactor this too, right? * where defined and used in singlediode_methods 6 times * ENH refactor pvsystem.estimate_voc too, also update docs to numpy style * ENH refactor vd->diode_voltage in bishop88 * ENH: TST: refactor reshaping conditions for _array_newton * test for size and shape in pvsystem not necessary for _array_newton and were redundant, so move them to singlediode_method, and only need np.vectorize for brentq for now * add get_size_and_shape which combines the redundant test conditions * separte imports for brentq and newton * use partial to set tol, maxiter, etc. * add _array_newton to tools.py * ENH: TST: make brentq vectorized always * replace and combine two *_x_from_y with a single function to do both bishop88_x_from_y(..., method='newton') * move reshaping back to pvsystem * change _get_size_and_shape to private method * add numpy style docstrings to bishop_x_from_y() functions * use current instead of "i" and voltage instead of "v" for args in bishop_x_from_y() * ENH: TST: can't vectorize brentq because it treats args as array * instead of each element of args as an array, ie: it loops over each item in args but objective requires _all_ of the items, we only want it to loop if each or any of the items in args are an array * fix don't import brentq twice, and don't import _array_newton as newton so it will keep working even after scipy-1.2.0 is released * need to vectorize brentq in each bishop88_x_from_y() by breaking the args out individually like they were originally * BUG: TST: use "brentq" instead of "gold" * use partial to set singlediode search method to brentq, newton, ... * add shape, size reshape logic back to pvsystem * use bishop88_x_from_y methods that combine all search methods * fix vectorized brentq functions to include i or v, and voc_est, and pass i or v to the objective, don't let it be set by locals, ditto for voc_est * fix newton need to set initial guess shape, and if using broadcast_to, then copy to get a new instance, or else can't update the guess * TST: change test fixtures with nonsensical values in quadrant four * several test_i_from_v_from_i and test_i_from_v test fixtures had voltages of -299.75 which would never be used in any test, and can only be solved using the lambertw maybe or the newton, but will always fail the brentq because it is bounded in quadrant 1 from zero to voc. * add tests for brentq and newton for test_v_from_i and test_i_from_v * also specify method in the test for graceful failure of mismatched arrays (ValueError: can't broadcast) to test newton and brentq explicitly * set default in pvsystem to 'lambertw' - let's take it slow * fix issue with newton i_from_v, cannot use voltage for initial guess and have it as part of the objective function because they have the same references, so must make a copy of it, call it v0 * also if max size > 1 then make sure all args are numpy arrays so that broadcasting errors are gracefully raised * TST: fix mpp tests * in floats, array, and series change default to 'brentq', and 'fast' to 'newton' * change IL list [7, 7] to an array, easy way out for now * add missing 2nd test comparing newton in test_mpp_series * fix randomly wierd indent * in pvsystem.singlediode() change default method to 'lambertw', baby steps for now, and update documentation change 'fast'->'newton' and 'gold'->'brentq' * in pvsystem.mpp change default method from 'gold'->'brentq' * use partial and make mpp_fun from bishop88_mpp and method.lower * revert changes that moved making dataframe to singlediode_methods, and wasn't working, move it back to pvsystem, and create a datafram if the photocurrent is a series * combine *_mpp() functions together to make bishop88_mpp(..., method), defaults to 'newton', and add docstring with numpy style * remove "recursive" call for vectorization, and use same pattern from other bishop88_*() methods for both brent and newton * add a stopgap *_mpp shortcut for slowway() and fastway() methods * TODO: fix slowway and fastway, and use np.asarray in voc_est * ENH: clean up last little bit * remove slowway and fastway * ENH: BUG: TST: DOC: remove all traces for "fast" or "slow" * remove "faster_way" and "slower_way" from test_singlediode_methods, and use the pvsystem.singlediode(..., method=<'newton', 'brentq', ...>) * update docstring notes in pvsystem.singlediode to refer to methods as "newton" or "brentq" instead of "fast" or "slow" * remove the returns_nan()() wrapper, do we need it? seems to pass all tests without it. * don't need partial in *_from_*, just call singlediode_method.bishop88 * remove extra imports from singlediode_methods like wraps, OrderedDict, pandas, also remove TODO and returns_nan()() * wrap a long line? * remove the old slowway and fastway functions no longer needed * TST: BUG: fix the boolean mask numpy<1.14 bug in tests * also cast photocurrent as array in estimate_voc in case it isn't and help users get away with using sequences instead of arrays * ENH: TST: ignore .pytest_cache/ folder * DOC: MAINT: respond to review by @cwhanse , clean up docstrings * reword methods for pvsystem.singlediode simplify, move some content to notes sections below * move descriptions of methods to top in same order as given in methods docstring, add description of brentq, move description of bishop88 last * fix typo "optional" missing "o" * MAINT: update comment about mpp search algorithm * MAINT: update what's new for v0.6 * add missing R_s = 0 for estimate_voc * change docstring string literal to use triple double quotes per pep8 * add bishop88 to see also * add comment before lambertw like "calculate points on IV curve using lambertw" * change "use single diode methods" to be more expanatory, like "calculate points on the IV curve using newton or brentq" * change desoto parameters to sdm coeffs * better shorter simpler docstring for mpp() method kwarg: just "brent" or "newton" * note why is brentq default in pvsystem.mpp(), b/c it's an option to pvsystem.singlediode and brentq is guaranteed to converge * remove "Faster ways" * remove diode "one" * fix typos, "o" in optional * DOC: make sure docs render well * update what's new with formatted links to first_solar_spectral_loss and calcparams_desoto * newton is not a gradient descent method, ha! * replace fast and slow with newton and brentq * add note that new module singlediode_methods is for low-level functions * add low level singlediode_methods to api.rst with a note separating it from pvsystem * remvoe way faster from test_singlediode * backticks around method argument options in singlediode * add escapes to math directive in estimate_voc and singlediode, and wrap some long lines a little * shorten description of methods in *_from_* * use rubric to separate and title Notes in estimate_voc * use LaTeX math for gradients instead of code format * DOC: MAINT: rewording what's new with @cwhanse comments to make it clear * API: change bishop88 and estimate_voc to be used from singlediode_methods * update what's new and remove pvsystem.bishop88 and est_voc from api.rst * also remove note that they're listed in 2 places, just leave note about low-level functions to solve SDM * update imports in test_numerical_precision and pvsystem, remove func assignments from pvsystem, update docstrings * change module docstring in singlediode_methods to be generic * DOC: link to new singlediode_methods subfunctions in what's new * in see also in pvsystem.singlediode need full path to bishop88 * in singlediode_methods, add a line before FIXME for clarity * API: DOC: ENH: change pvsystem.mpp() -> max_power_point() * update api.rst and what's new, and test_pvsystem.py everywhere where it says mpp() * remove logging, LOGGER, timeing, and CLI for test_singlediode_methods * remove p from out in pvsystem.singlediode if ivcurve_npts is provided since not part of original api * so remove p from test_singlediode_methods.py too * add singlediode_methods to pvlib/__init__.py * just assign out once for all methods, outside of the if/else blocks in pvsystem.singlediode * since IDE complains, and future maintainer may not realize, initialize ivcurve_* with NotImplemented instead of None or nothing at all so this is explicit, you must calculate this or else * need to create values for i_sc, i_x, and i_xx in bishop88 methods to fill out * MAINT: address comments by @wholmgren * remove extra 'p' from out, in test_pvsystem * make tests in test_singlediode_methods more descriptive, add docstring and change "fast"->"newton" and "slow"->"brentq" * use suggested language for notes in max_power_point() * replace confusing lambdas with real function definitions that are easier to understand in bishop88_i_from_v and v.v., also add a comment explaining why this is necessary * remove import of tools * add FIXME and comment explaining that we need to remove _array_newton at some point in the future, and the reason why we have it here * MAINT: add comment to explain why we import brentq in try-except * MAINT: move lambertw methods to singlediode_methods.py * closes #497 * remove lines setting ivcurve points to NotImplemented, so that they raise an error if they are not set * replace nested code in if: else condition for lambertw with new private method "_lambertw" in singlediode_methods.py * need to break out returns from private methods, there are so many * move _golden_sect_DataFrame to pvlib.tools * replace nested code in if: else condition for lambertw with new private methods _lambertw_v_from_i and _lambertw_i_from_v in singlediode_methods.py * move _pwr_optfcn to singlediode_methods.py * MAINT: add docstring elaborating the numerical precision test * explain how to generate the high-precision test data by running the file from the command line * MAINT: use np.expm1(x) for exp(x) - 1 in bishop88 single diode method * closes #500 * also add a comment that we are creating some temporary values to make calculations simpler * also use more descriptive names instead of a, b, c, use v_star, g_sh, and g_diode * MAINT: replace boilerplate code for broadcasting newton array args * closes #498 * adds _prepare_newton_inputs(i_or_v_tup, args, v0) * also replace verbose comment with more descriptive one to copy v0 if it's an array to use for initial guess * MAINT: TEST: parametrize i_from_v and v_from_i for methods, atol * closes #499 * MAINT: remove import of functools.partial in pvsystem.py * replace partial functions v_from_i_fun, i_from_v_fun, and mpp_fun with full names, and extra keyword argument method=method.lower() everywhere * MAINT: wrap lines longer than 79 characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
git diff upstream/master -u -- "*.py" | flake8 --diffand/or landscape.io linting service.docs/sphinx/source/whatsnewfile for all changes.