Proposal: split up mismatch.py

[RDaxini]

Is your feature request related to a problem? Please describe.
I've been working with the spectrum unit of pvlib a fair bit recently for my GSoC project and I find that the pvlib/spectrum/mismatch.py file currently contains a range of functions, several of which are not directly related to spectral mismatch.

Current functions in pvlib/spectrum/mismatch.py:

pvlib.spectrum.get_example_spectral_response
pvlib.spectrum.get_am15g (deprecated v0.11, removed 0.12)
pvlib.spectrum.get_reference_spectra
pvlib.spectrum.calc_spectral_mismatch_field
pvlib.spectrum.spectral_factor_caballero
pvlib.spectrum.spectral_factor_firstsolar
pvlib.spectrum.spectral_factor_sapm
pvlib.spectrum.spectral_factor_pvspec
pvlib.spectrum.spectral_factor_jrc
pvlib.spectrum.sr_to_qe
pvlib.spectrum.qe_to_sr

Describe the solution you'd like
I am wondering whether it might help organise the functions more clearly if we were to split up this file into several subpackages, still within pvlib/spectrum.

One suggestion:

1. pvlib/spectrum/mismatch.py
   --> covers the effect on PV performance; calculations of spectral mismatch (atmospheric, field, indoor)

2. pvlib/spectrum/spectral_irr.py
   --> covers the outdoor meteorological side of things; all spectral irradiance calcs, e.g. get_reference_spectra, possibly move spectrl2 into here as well

3. pvlib/spectrum/spectral_resp.py
   --> covers the PV characteristics side of things, e.g. sr_to_qe / qe_to_sr calculations, get_example_spectral_response

I think this framework fits with what we have currently and, in terms of future development, I see scope for expansion of spectral_irr.py in particular to include for example, function to calculate spectral indices such as the average photon energy. I can see mismatch.py being expanded to include variants of the standard spectral mismatch factor, or alternatives such as the (weighted) useful fraction.

Describe alternatives you've considered
Could just rename mismatch.py entirely to broaden its scope, but I think creating a few files here instead would be better for organisation and future development.

Additional context
The suggestion above is just one idea to illustrate what I am thinking about. I am keen to hear other views on this idea.

1. Is splitting pvlib/spectrum/mismatch.py a good idea?
   If yes:
2. Alternative categorisation structure?
3. Alternative file names?

I'd be happy to work on this if we reach a consensus. I'm looking forward to hearing your views.

[mikofski]

You mean spectrum.py? Why not make it a sub-package?

• pvlib/spectrum/mismatch.py
• pvlib/spectrum/irradiance.py
• Etc

I’m -1 on using a pvlib sub package above spectrum called mismatch because it has many meanings, like electrical mismatch or array level mismatch etc

another idea instead of mismatch usepvlib.spectral.shift? “Spectral shift” is also sometimes used

[RDaxini]

I’m -1 on using a pvlib sub package above spectrum called mismatch because it has many meanings, like electrical mismatch or array level mismatch etc

@mikofski I don't mean to create a package above spectrum called mismatch. I agree that would not be a good idea. I mean splitting up the existing mismatch.py into several subpackages, all definitely still within spectrum.

• pvlib/spectrum/mismatch.py
• pvlib/spectrum/irradiance.py
• Etc

This is also what I meant. So what we have currently is:

• pvlib/spectrum/mismatch.py
• pvlib/spectrum/spectrl2.py

and what I was thinking about is taking some functions out of mismatch.py and putting them into some new subpackages (possible new subpackage names in the original post) still within within pvlib\spectrum

I hope this makes my suggestion clearer than in the original post 😅
(I have revised the original post by adding pvlib/spectrum to clarify this too)

[echedey-ls]

I agree on rearranging things.

I would keep spectral mismatch factors in mismatch.py and put groups 2 & 3 into a new responses.py file. Spectral2 is good in it's own file IMO, specially for maintenance purposes.

Another point, do we need deprecations for this? I doubt anybody includes from the mismatch.py file namespace since all is already available under pvlib.spectrum

[RDaxini]

Thanks for your suggestions @echedey-ls

and put groups 2 & 3 into a new responses.py file

On this point, what is your thinking behind combining the groups?
I lean more towards separate groups since I think the functions would be sufficiently distinct from one another. Group 2 is based on spectral irradiance data, while group 3 is based on the PV device. Just for example, I don't think a function to calculate a parameter such as the average photon energy (to characterise a spectral irradiance distribution) would fit in a responses.py package, but a spectral response curve or associated response curve analysis functions would.

Another point, do we need deprecations for this?

Good point. I am not sure --- @AdamRJensen @kandersolar ?

[echedey-ls]

On this point, what is your thinking behind combining the groups?

Mainly because the main topic seems that they are related to calculating spectral mismatch or response from an SR and a spectrum. Also, because the prefix spectral in spectral_irr.py and spectral_resp.py seems redundant since those files will be under pvlib.spectrum. And from the contractions irradiance and response, irradiance is somewhat already taken:

from pvlib import irradiance
from pvlib.spectrum import irradiance

This wouldn't work if a file is named irradiance.py under spectrum/. But yeah, we could find some other name I guess.

And then we come to the first point, which is why split things that are going to be used for a common purpose.

Anyway, not a strong opinion; I'm grateful that the base code gets broke down into smaller and maintainable pieces.

[AdamRJensen]

Because the names of these sub-files aren't seen by the user (functions are imported from pvlib.spectrum), then we're more lenient on using longer filenames. Another response for the prefix, as pointed out by @echedey-ls, is that there's already a file named irradiance.py.

Given that the functions are advertised in the documentation as imported from pvlib.spectrum, then we can justify not having a deprecation cycle for the filename changes.

As we discussed during today's GSoC group meeting, then I think the preferred naming is:

• spectral_response.py
• spectral_irradiance.py
• spectral_mismatch.py

Also, don't forget to update the __init__.py file.

[cwhanse]

@AdamRJensen can you write a one sentence description of each of these modules? For those not at the GSoC meeting.

[AdamRJensen]

@AdamRJensen can you write a one sentence description of each of these modules? For those not at the GSoC meeting.

@RDaxini could you do that? I'm back in the saddle peddling my way through Greece

[RDaxini]

So in the GSoC meeting we talked more specifically about the naming and combining subpackages or not. Since the names are similar to the original post suggestion, my understanding is the content is broadly similar too:

• pvlib/spectrum/spectral_mismatch.py
  --> covers the spectral effect on PV performance, ie models to calculate spectral mismatch
• pvlib/spectrum/spectral_irradiance.py
  --> covers the meteorological ('off/non-PV') aspect, ie spectral irradiance calculations such as get_reference_spectra, or new functions such as get_ape (average photon energy)
• pvlib/spectrum/spectral_response.py
  --> covers the PV side of things, e.g. sr_to_qe/qe_to_sr calculations, get_example_spectral_response

ps welcome to join our fun meetings next time @cwhanse :D

[cwhanse]

Thanks, that clarifies for me the difference between spectral_mismatch and spectral_response.

I'll let you know if I need another meeting :)

[wholmgren]

I recommend against repeating "spectral" in the module name - they're already in the spectrum namespace! There's no conflict unless a user tries to import in the way that @echedey-ls demonstrated and the answer for that user is "don't do it that way, instead do something like import pvlib.spectrum.irradiance as spectral_irradiance". And don't forget that many of the function names have "spectral" in them too.

[RDaxini]

Should we vote?
Thumbs up this comment if you're for the spectral_ prefix, thumbs down this comment if you're against the prefix?