Allow users to interact with root finders parameters

[echedey-ls]

• Closes Allow user to set tol and maxiter for singlediode newton method #1249
• I am familiar with the contributing guidelines
  - [x] Updates entries in docs/sphinx/source/reference for API changes.
• Tests added
• New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
• Pull request is nearly complete and ready for detailed review.
• 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`).
• Maintainer: Appropriate GitHub Labels (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

Implementation details

I decided not to follow @cwhanse guidelines, since those two methods have some minimal difference in the arguments xtol (brentq) and tol (newton). I think PVLIB is not responsible for such a use case where it needs to name them the same.

kwargs are passed to the root finders, checking that values for newton method are present and, if not, setting default values to them. This values are from before this PR. See this line of code.

No error checking is made to kwargs. If any invalid arg is passed, then Python raises an error. See comment.

Docs links

bishop88_i_from_v
bishop88_v_from_i
bishop88_mpp

[adriesse]

I support the initiative. I would be inclined to send **kwargs to optimizer so any other available options can be used. You could set pvlib defaults in this dictionary if they are absent and needed (or override incompatible options).

[echedey-ls]

Thanks for raising that point @adriesse !
I've updated the code, but I still have to figure out the tests.

[adriesse]

Maybe try to describe the test objectives in English here to start? Maybe also play with the kwargs and see if interesting exceptions pop up?

[echedey-ls]

Thanks for the insight @adriesse
If an incorrect arg is passed to a solver the following error is raised:

Exception has occurred: TypeError
newton() got an unexpected keyword argument '_inexistent_param'
  File "C:\...\pvlib-python\pvlib\singlediode.py", line 297, in bishop88_i_from_v
    vd = newton(func=lambda x, *a: fv(x, voltage, *a), x0=v0,
  File "C:\...\pvlib-python\docs\examples\test.py", line 73, in <module>
    bishop88_i_from_v(0, **bishop88_args, method=method, **kwargs)
TypeError: newton() got an unexpected keyword argument '_inexistent_param'

From the traceback I feel it would be easy for a user to spot the error, I mean, if they are using kwargs they are reading the docs and possibly scipy too.

I consider bishop88_.* implementation, tests and docs finished. Let me know any changes you would like.

[cwhanse]

thanks @echedey-ls

[kandersolar]

Looks good overall @echedey-ls! One minor edit and a bigger comment on testing below.

[adriesse]

This technically isn't a comment on the proposed changes, but whenever you touch an older piece of code other questions tend to come up. In this case:

• Can it really take 100 iterations be needed to find a point on an IV curve to a precision of 1e-6? I-V curves don't behave that badly do they?
• Would it would be more appropriate to set rtol instead of tol since the diode model parameters could represent a single cell, a module, or even a string of modules? In any case, an absolute precision of 1e-6 is probably not routinely needed even for single cells.

[echedey-ls]

Thanks for bringing that up @adriesse
That's essentially what issue #1249 is about. The author claims look good, but no code, range of tested data, cell materials, or evidence is provided, and instead of relative tolerance, absolute one is used.
We should maybe ask them how they tested that, or provide the code.

[adriesse]

I guess that allowing people to play with the options is only one thing; the other is to provide feedback on the effect of those options. Does the full_output option carry through ok?

Here's some more scope creep for fun: support the 'secant' method option by running newton with fprime=None.

[echedey-ls]

Does the full_output option carry through ok?

Of course not. But that result does not seem very important for an end user. Maybe to determine the pvlib defaults it is. Especially:

r : RootResults, optional
Present if full_output=True and x0 is scalar. Object containing information about the convergence. In particular, r.converged is True if the routine converged.

We can raise an error if that is passed, or return those results too (doesn't seem very difficult, just unpacking and a conditional return).
I'm more inclined to do the first one, since IMO the latter is over engineering and out-of-scope of the bishop88_.* functions. It's up to you what you consider adequate.

[adriesse]

I don't mind if you keep the changes minimal in this PR. I'm just throwing in some ideas that could make it potentially more useful.

[echedey-ls]

@adriesse I ended up doing your suggestion so that's easier to determine the optimal default params for the method(s) later.

I feel tests and docs are right. I think this PR can be reviewed now.

[kandersolar]

Right now this docstring entry is inconsistent with the actual function behavior. The difference is whether the function is used as:
bishop88_i_from_v(..., method_kwargs={'tol': 1e-5}) (matches the docstring)
or
bishop88_i_from_v(..., tol=1e-5) (matches the function signature)

I think I somewhat prefer the first one. @adriesse?

[echedey-ls]

Good point. It looks a bit weird how kwargs are documented. Example provided from numpydoc:

**kwargs : dict, optional
    Extra arguments to `metric`: refer to each metric documentation for a
    list of all possible arguments.

However, in PVLIB (taken from pvlib.location.Location.get_clearsky) it is usually

kwargs
            Extra parameters passed to the relevant functions. Climatological
            values are assumed in many cases. See source code for details!

This stackoverflow post does give some more insight that could be useful.

I like the function signature way of passing parameters, given they are well documented, and document kwargs with **kwargs without type definition.
If no objections are made, I'll add some use examples both for kwargs and full_output=True.

[echedey-ls]

I like the function signature way of passing parameters, given they are well documented, and document kwargs with **kwargs without type definition.

I didn't justify my opinion. Right now, **kwargs is well spread in PVLIB (well, in docs it usually is kwargs, without preceding **) to pass parameters to other functions, so I feel the dict variant a bit disruptive in that sense. I added the asterisks cause I think they hold an important meaning. Of course I don't have a strong opinion and if you tell me otherwise I'll change it without problems.

Something that puzzles me a bit is using full_output inside the functions, making it not just a keyword arg passed to the root finder. However, IMO this is a right approach for now. Adding a parameter for that seems redundant.

[adriesse]

I thought there might be others with opinions about this...

There's a similar situation in scipy least-squares. I think having the extra arguments that will be passed to another function reside inside a dictionary (or a list of args, like newton does) clearly separates them from those that are used within.

[echedey-ls]

Okay, I've changed the behaviour although that still feels like the pattern for a user-provided function.

tests (with few unrelated exceptions I believe), doctest examples are passing and all seems good to me. Do you mind having a look specially at the formatting of docs?

[adriesse]

I guess choosing the method name is an indirect way of giving a user-provided function. :)
I'm not that good at the doc formatting so I'll defer to others.

[echedey-ls]

Can you have a look at the docs and tell me if anything is missing or not needed? They are linked in the body of the PR.

[cwhanse]

Can you have a look at the docs and tell me if anything is missing or not needed? They are linked in the body of the PR.

Docs look fine to me.

[kandersolar]

Docs look good to me too. A couple minor suggestions below, otherwise I think LGTM.

One kind of weird thing: using np.vectorize in the method='brentq' branch results in an array of scipy result objects, while the result for method='newton' is a single vectorized result. I don't think anything needs to change about this PR, just wanted to point it out.

[echedey-ls]

Thanks for suggesting the pythonic way to do that.

One kind of weird thing: using np.vectorize in the method='brentq' branch results in an array of scipy result objects, while the result for method='newton' is a single vectorized result.

Good point. Honestly, I feel the solution was a bit over engineered, but that it will serve the purpose correctly.

Thanks everyone for the reviews.

[adriesse]

the solution was a bit over engineered

Not unusual! :)

[kandersolar]

Thanks @echedey-ls! This is a nice addition to pvlib.