ENH: allow user to specify the scaling of singlediode ivcurve-pnts or a specify an array, series, or sequence

[mikofski]

This idea also came up somewhere in #409 or #410. Currently the singlediode ivcurve_pnts takes the following according to docstring:

ivcurve_pnts : None or int, default None
    Number of points in the desired IV curve. If None or 0, no
    IV curves will be produced.

idea 1:

In addition let the user specify a sequence. Should it be sorted? Which values do they correspond to, I, V or Vd? IMO we should sort and scale them to the max value, and then multiply by Voc to keep them in the 1st quadrant.

idea 2:

Add another argument like ivcurve_scale which could be 'log' or 'linear' or other? IMO the default should be a log scale

Other ideas? Yea or Nay?

[cwhanse]

Idea 1 - use keyword arguments maybe. V is most commonly used, I believe.

Idea 2 - I'm ok with this as an optional argument. I'd default to log (natural log) of V.

[adriesse]

I would suggest providing any options you can think of since this is kind of a do-it-all function. Maybe even accept a list of V or I values (fractions of Voc and Isc).

[wholmgren]

I think @adriesse makes a good point.

I also think we'd be better off in the long run if we moved this functionality from singlediode to a dedicated function. Maybe rename singlediode to singlediode_5points and make a new singlediode_curve function.

[cwhanse]

I'm working on this issue and would appreciate any reactions to this addition of a kwarg voltage in pvsystem.singlediode.

[mikofski]

I would be slightly more in favor of reusing ivcurve_pnts as voltages if a sequence is specified, instead of ignoring it if voltages is specified in as well. IMO this might lead to confusion or at least make it prone to oversight errors. I believe having a single keyword that dispatches differently based on duck-typing makes oversight errors less likely, because the intent is more closely matched to the type, ie: a sequence could really only mean voltages.

try:
    voltages = iter(ivcurve_pnts):
except TypeError:
    voltages = None
else:
    ivcurve_pnts = None

This works for all combinations:

ivcurve_pnts = None  # -> voltages=None, ivcurve_pnts=None
ivcurve_pnts = 123  # -> voltages=None, ivcurve_pnts=123 (also works if x=0)
ivcurve_pnts = [1, 2, 3]  # -> voltages=<list_iterator>, ivcurve_pnts=None
ivcurve_pnts = range(100)  # voltages=<range_iterator>, ivcurve_pnts=None 

then either iterate over voltages or if needed call list(voltages) or tuple(voltages) to expand the sequence

[cwhanse]

I thought the same but got stuck on the case when ivcurve_pnts is a singleton. What does ivcurve_pnts = 5 mean? 5 points, or evaluate at voltage = 5? That's when I added the second kwarg.

[wholmgren]

I still think this function is a mess and should be broken up into smaller pieces.

[cwhanse]

I still think this function is a mess and should be broken up into smaller pieces.

Maybe I was a bit quick to thumbs up. Is the mess in pvsystem.singlediode because of the difference in the signatures for the lambertw and bishop88 functions?

[wholmgren]

Is the mess in pvsystem.singlediode because of the difference in the signatures for the lambertw and bishop88 functions?

That certainly doesn't help the situation, but my main complaint is that the function is supposed to do two different things: 1. calculate the 7 special points on the IV curve and 2. arbitrary points on the IV curve. The fact that the returned dict keys are different depending on the inputs is an indication that the API is not good. It also has some handling for time series that seems like a bad fit.

[cwhanse]

That certainly doesn't help the situation, but my main complaint is that the function is supposed to do two different things: 1. calculate the 7 special points on the IV curve and 2. arbitrary points on the IV curve.

What behavior would be expected from PVSystem.singlediode? As a user, I wouldn't want to switch methods to get both the typical 5 points and a list of other points. I'd want one method that can provide both.

The fact that the returned dict keys are different depending on the inputs

Agree, this should be addressed.

It also has some handling for time series that seems like a bad fit.

Yes. And the if/then language in the Notes seems an obtuse way to describe the built-in spacing in points.

[mikofski]

What does ivcurve_pnts = 5 mean? 5 points, or evaluate at voltage = 5?

I think it should means 5 points, and perhaps ivcurve_pnts = [5] should mean evaluate the IV curve at a voltage of 5[V]

As a user, I wouldn't want to switch methods to get both the typical 5 points and a list of other points.

me neither

[adriesse]

As a (non-typical) user I'm happy calling the bishop88 functions as often as necessary to get what I need. And I never seem to need more than 3 of the "typical 5" points.

In general I don't see any problem having two or more separate (wrapper) functions that each give a curve, 5 points, or some other useful output combination.