Return dictionaries instead of tuples

[Peque]

I found it a bit inconvenient to have fit_sdm_cec_sam() return a tuple. I think returning a dictionary with ['I_L_ref', 'I_o_ref', 'R_sh_ref', 'R_s', 'a_ref', 'Adjust'] keys would be much more convenient and safe.

The same goes for calcparams_cec(), which could return a dictionary with ['photocurrent', 'saturation_current', 'resistance_series', 'resistance_shunt', 'nNsVth'] keys instead.

I think that is more Pythonic (explicit is better than implicit and readability counts).

Also, more convenient API-wise:

cec_parameters = fit_sdm_cec_sam(...)
sd_parameters = calcparams_cec(..., **cec_parameters)
curve_info = singlediode(..., **sd_parameters)

The tests could be simplified as well. They currently look like:

I_L_ref, I_o_ref, R_sh_ref, R_s, a_ref, Adjust = ivtools.fit_sdm_cec_sam(...)
expected = pd.Series(get_cec_params_cansol_cs5p_220p['output'])
modeled = pd.Series(index=expected.index, data=np.nan)
modeled['a_ref'] = a_ref
modeled['I_L_ref'] = I_L_ref
modeled['I_o_ref'] = I_o_ref
modeled['R_sh_ref'] = R_sh_ref
modeled['R_s'] = R_s
modeled['Adjust'] = Adjust
assert np.allclose(modeled.values, expected.values, rtol=5e-2)

They could look like:

modeled = ivtools.fit_sdm_cec_sam(...)
expected = get_cec_params_cansol_cs5p_220p['output']
assert modeled == pytest.approx(expected, rel=5e-2)

[kandersolar]

For reference, tuples vs dicts was discussed briefly (with no immediate resolution) in this thread: #784 (comment)

[Peque]

PD: Since what I am proposing is a breaking change... I also think that reducing the number of parameters in a function is good (i.e.: curve_info = singlediode(..., sd_parameters=sd_parameters)), where sd_parameters is a variable containing all the SDM parameters. Although this could also be applied with a tuple instead of a dict.

[mikofski]

My preference would be to use keyword arguments instead, eg: curve_info = singlediode(..., a, b, c, d) = singlediode(..., **sd_parameters) where sd_parameters = {'a': 12.3, 'b': 4.56, 'c': 0.789, 'd': False}. I believe there are a couple of reasons why this could be more Pythonic:

1. The keyword arguments are explicit. If an argument is misspelled or missing, the function will raise an exception
2. Whereas I believe a dictionary argument would be implicit. It requires documentation to tell the user what the required keys should be. This is potentially error prone, one could mistype a key or miss one.
3. Using keywords allows the user all options, pass the dictionary using splat, as keywords, or positional.

[wholmgren]

In my experience maintaining pvlib, it's been easier to deal with things that require order (positional args and returned tuples) than things that require labels (keyword args and returned dicts or DataFrames). I do use a lot of dicts, Series, and DataFrames in my work, so I certainly appreciate their general value. It just seems they're harder for a more general purpose library. As I mentioned in the linked issue, I'd really like to read a blog post about this kind of API design question in scientific computing.

It's also not clear to me that you'd actually need to switch to keyword arguments to achieve the input simplification you're after. Examples showing that it can be done with positional arguments:

In [1]: def f(a, b):
   ...:     print(a, b)
   ...:

In [2]: f(b=2, a=1)
1 2

In [3]: d = dict(a=1, b=2)

In [4]: f(**d)
1 2

In [5]: t = (1, 2)

In [6]: f(*t)
1 2

The test simplification could work with a tuple too:

assert (1., 5.) == pytest.approx((1., 5.))

[mikofski]

Will you explained what I was trying to say, but much better, thanks!

[Peque]

@mikofski I have no strong opinion on packing/unpacking parameters. 😊

@wholmgren Yeah, tests simplification can be done either way, I should not have brought it to this issue. 👍

@wholmgren In my opinion positional args should be enforced then. That is possible in Python 3.8, but not before. Also, positional arguments must come before keyword arguments, and that may require further API modifications.

In example:

# Assuming `cec_params` is a tuple...
cec_params = fit_sdm_cec_sam(...)
# This works fine
calcparams_cec(
    1000,
    25,
    0.004,
    2.5,
    *cec_params,
    ...,
)
# This does not
calcparams_cec(
    effective_irradiance=1000,
    temp_cell=25,
    alpha_sc=0.004,
    a_ref=2.5,
    *cec_params,
    ...,
)

You can't just use keyword parameters before the cec_params expansion and, in my opinion, that makes the code less legible/explicit and the API less convenient/friendly to use. So, in my opinion it would be better to change the API to have *cec_params first, and then effective_irradiance, temp_cell, ....

To summarize, I think returning a dict is better and I do not mind that much about params=dictionary vs. **dictionary on function calls. But your project, your rules! Feel free to close this. 😜

[adriesse]

A worthy discussion.

[mikofski]

ICYMI: @kanderso-nrel and @CameronTStark - could be useful to get your opinions on this, if you care to weigh in.

[kandersolar]

On the subject of passing groups of values into a function as a unit vs individually -- if the values are coherent enough to interact as a group in several parts of the API, I wonder if they would be better grouped as a simple object like scipy's OptimizeResult instead of a dictionary.

For positional-only arguments, I don't think it's a good idea at this time. For one, it would require a big bump in minimum python version, and for two I don't think pvlib satisfies any of the use cases listed in PEP 570.

For returning dicts vs tuples... a 6-element tuple is a bit unwieldy. On the other hand, returning a dictionary for two values would seem silly. I notice fit_sdm_desoto returns a dictionary. But taking a step back, it seems like there will always be some complexity to the plumbing of these low-level interfaces, which suggests maybe they need a high-level interface. Just throwing out an idea, what about some sort of "PVDevice" class that behaves like this?

device = fit_device(v_mp, i_mp, ... model='cec')
# or
device = Device(I_L_ref, ..., model='cec')
# calls calcparams_x and singlediode under the hood:
curve_info = device.ivcurve(effective_irradiance, ...) 

[CameronTStark]

@wholmgren 's description of the situation is good although I would prefer to have a dictionary on the output if there are more than two values being returned. This makes it possible to enable easy transfer of the output of one function into another without careful positioning.

In [1]: def outputs_dictionary():
         ...:     return dict(this="foo", that="bar", other_thing="baz")

In [2]: def uses_output_dictionary(first_arg, that, other_thing, this, last_arg):
         ...:     print(this, that, other_thing)

In [3]: example_dictionary = outputs_dictionary()

In [4]: example_dictionary
Out[4]: {'this': 'foo', 'that': 'bar', 'other_thing': 'baz'}

In [5]: uses_output_dictionary(42, **example_dictionary, last_arg=1337)
foo bar baz

In [6]: uses_output_dictionary(last_arg=1337, **example_dictionary, first_arg=42)
foo bar baz

This is useful in interactive data analysis as well as in programmatic scripting to unify the API. Having sensible defaults for these arguments, when possible, makes this even easier and is also useful in enabling new users a path forward when they'd otherwise have no idea what value to insert.

[wholmgren]

I don't think we're going to get any farther arguing in the abstract. Can the proponents of an API change provide a list of functions for which they think it should change? Are we only talking about the single diode functions?

While we're at it, we might also consider functions that return labeled data (either as DataFrames or dicts) that could simply be ordered instead.

Having sensible defaults for these arguments, when possible, makes this even easier and is also useful in enabling new users a path forward when they'd otherwise have no idea what value to insert.

I'm not aware of any pvlib functions that require input for which there exists reasonable defaults, but they exist then we should address them in separate issues.