Adapts irradiance.isotropic to return_components framework.

[ramaroesilva]

• Closes Consider allowing return_components in all transposition model functions #1553
• I am familiar with the contributing guidelines
• Tests added
• Updates entries in docs/sphinx/source/reference for API changes.
• 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`).
• New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
• Pull request is nearly complete and ready for detailed review.
• Maintainer: Appropriate GitHub Labels (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

[ramaroesilva]

For now, I only updated irradiance.isotropic as a testbed for the proposed changes.

Made use of the content from perez and haydaviesdocstrings, which were not fully consistent. Also, added the poa_ prefix to the outputs as suggested by @markcampanelli here

If this seems good to everyone, I can replicate this for all functions (adding the return_components and making documentation more consistent across fuctions, whenever each is needed).

[kandersolar]

Thanks @ramaroesilva! I agree it makes sense to trial it on one function first. Let's wait to do the others until #2529 has some more discussion.

In the meantime, some minor comments below.

[kandersolar]

Minor note: no need to use OrderedDict in new code (#1684)

[kandersolar]

Suggested change

	or not. If `False`, ``sky_diffuse`` is returned. If `True`,
	or not. If `False`, ``poa_sky_diffuse`` is returned. If `True`,

Assuming this is in reference to the names in the Returns section

[kandersolar]

Update with poa_ prefix

[ramaroesilva]

Once more people give feedback on the documentation structuring of this example, I will continue developing the other transposition functions taking into account the OrderedDict comment. All poa_ related changes will move to a separate PR.

[cwhanse]

I would prefer to return both poa_sky_diffuse and diffuse_components, if return_componets is True.

[ramaroesilva]

Sounds good to me. This "one or the other" is what is implemented in perez at the moment.

For clarity, do you mean returning two dictionaries, which makes the number of outputs depend on return_components, or having still one dictionary buth with an additional sky_diffuse key?

[cwhanse]

I'm thinking a tuple with two dicts, when return_components is True.

[ramaroesilva]

@cwhanse I just noticed that in my latest commit I included in the diffuse_components dict the sky_diffuse as one of the keys. Would this - having a single-dict with components and their sum - make sense?

If instead we have two dictionaries as originally proposed, the one having the sky_diffuse will have a single key... How would we call the variable and the key without being repetitive? total_diffuse["sky"]?

[cwhanse]

Apologies, I misspoke. Return a tuple (sky_diffuse, components) where sky_diffuse is the current numeric (Series, array, float) and components is a dict.

[cwhanse]

Edited: returning a tuple requires the user to change from

sky_diffuse = isotropic(...)

to

sky_diffuse, _ = isotropic(...)

I was (wrongly) thinking that only the first element of the tuple would be assigned to sky_diffuse. Not thinking clearly.

So now I'm not so sure that returning a tuple is a good idea. Still, it seems better than requiring the user to change to

out = isotropic(...)
sky_diffuse = out['sky_diffuse']

[ramaroesilva]

Thanks for clarifying and thinking things through.

Going back to the start:

I would prefer to return both poa_sky_diffuse and diffuse_components, if return_componets is True.

I just remembered that in my latest commit the sky_diffuse is included within diffuse_components, following the workflow used in perez and haydavies. So what we are discussing here is in fact if we should change the current modus operandi of transposition models with return_components.

While what you propose is v0.13.1-friendly for the models not yet returning return_components, it would break previous code for those that already do. Having this said, I would suggest moving this specific topic to a separate issue and raise some discussion aiming for v0.14.

What do you think? If agreed, can you can raise a new issue like @kandersolar did in #2529?

[adriesse]

I just remembered that in my latest commit the sky_diffuse is included within diffuse_components, following the workflow used in perez and haydavies. So what we are discussing here is in fact if we should change the current modus operandi of transposition models with return_components.

All the irradiance component sets I looked at include both components and sums of components. I think the cleanest way would be (would have been) to include only the lowest level and leave the summation up to the user.

[ramaroesilva]

Any comments about my question here? This code is used in perez.

[cwhanse]

Yes, should preserve the index from dhi

[ramaroesilva]

Would adding this index bit to the diffuse-component-including perez and haydavies functions be considered a breaking change?

[cwhanse]

I don't think that's a breaking change. pvlib strives that a function returns the same data type as the inputs. That's one reason we test functions with different input types. If those functions are returning e.g. Array for Series input, I'd regard that as a bug that needs fixing.

In perez the index wasn't explicitly set because the upstream calculations resulted in sky_diffuse being a Series. I think it's better to be explicit, just in case the calculation uses some numpy function that casts Series to Array.