Migrate spectral modifier functions to pvlib.spectrum

[kandersolar]

• [ ] Closes #xxxx
• 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.

As suggested in #1062 (comment) and maybe elsewhere, this PR moves the functions for First Solar and SAPM spectral modifiers to the pvlib.spectrum.mismatch module.

I also thought as long as we are moving things, it would be good to rename sapm_spectral_loss to sapm_spectral_correction. Currently, the two functions are renamed to just spectrum.sapm and spectrum.first_solar. Now renamed to spectrum.spectral_factor_firstsolar and spectrum.spectral_factor_sapm Do others agree with this change? Should pvlib.pvsystem.PVSystem.sapm_spectral_loss be renamed as well?

[adriesse]

Good move!

Still thinking about the names and wondering what naming patterns they conform to and which patterns are currently gaining or losing popularity. Subspecies (?) is often at the end.

[adriesse]

I'm not in favor of shortening the function names to just the author names. I though we had some other discussion recently where that was discouraged actually.

[mikofski]

I had a similar thought recently, thanks for pulling this together!

[kandersolar]

I'm not in favor of shortening the function names to just the author names. I though we had some other discussion recently where that was discouraged actually.

My thought was that function names in other modules don't include the type of the model, e.g. it's not iam.sapm_reflectance or somesuch, so it makes sense to remove spectral_correction from these names. But that's a separate consideration from whether it's desirable to choose sapm and first_solar as the model names here. Suggestions for alternatives are welcome :)

[cwhanse]

I don't know if this scales, but maybe we can reach out to authors and ask them what they'd like the function name to be.

I agree with shortening the function names when the module conveys the function purpose, but, that creates a pit for new users to fall into:

from pvlib.iam import sapm
from pvlib.spectrum import sapm

etc.

Another thought to toss out: we could add a publication year to the function name to create space for e.g. another model from First Solar.

[adriesse]

I think the year may be added on occasion, but I don't think this is a global solution.

There are few modules/namespaces as focused and conveniently short as iam; because of these two characteristics it is not onerous to import iam and use iam.author(). We could try to create more of those, but again it would not be a global solution.

Spectrum does not contain only one type of function, so I think the functions need a suitable prefix. The latest addition to this module got a nice long name: calc_spectral_mismatch_field so if we take that as a starting point the new ones could be:

calc_spectral_factor_sapm
calc_spectral_factor_firstsolar

I somewhat hesitate to use spectral_mismatch in the names of the empirical models or approximations.

I'm also not sure about the calc_ prefix because it doesn't seem to add much information, but I would go with the flow on that one. Maybe we could introduce a prefix est_ for functions that merely estimate things. :)

[cwhanse]

spectral_factor_sapm
spectral_factor_firstsolar

These work for me.

I somewhat hesitate to use spectral_mismatch in the names

Ditto.

[kandersolar]

I have updated to spectral_factor_sapm and spectral_factor_firstsolar, but still grumble to myself on the basis of there being no irradiance.transposition_perez or irradiance.decomposition_erbs ;)

[adriesse]

I'll be anticipating some PR's with renames then!

[kandersolar]

Looking back through the discussion, it seems like the current state of the PR is acceptable to those who expressed opinions on the matter. I'll go ahead and merge to prevent it from languishing again.

Codecov failure can be ignored.