pvlib / pvlib-python

A set of documented functions for simulating the performance of photovoltaic energy systems.
https://pvlib-python.readthedocs.io
BSD 3-Clause "New" or "Revised" License
1.19k stars 998 forks source link

"spectral irradiance" variable defintion/standardisation suggestion #2150

Open RDaxini opened 3 months ago

RDaxini commented 3 months ago

Is your feature request related to a problem? Please describe. The user guide definition of variables and symbols section (found here) does not currently have a standard definition for "spectral irradiance".

Describe the solution you'd like

Additional context

Suggestions: (updated from comments) si, (si_sun, si_ref...) spectral_irr e spectrum spectra spectral_irradiance _spectral (suffix, e.g. ghi_spectral)

e_sun and e_ref make sense in calc_spectral_mismatch_field() but, for a function that doesn't require the _sun/_ref specification, e alone might be ambiguous. I like spectral_irr as it's clear, but it is potentially too long? A shorter alternative is si.

Thoughts? Other suggestions?

cwhanse commented 3 months ago

Any irradiance quantity could have a corresponding spectral variable. Broadband irradiance (a single number) is an integration over wavelength of a vector of wavelength-dependent values. I think the plane and nature of the measurement matter. Perhaps "_spectral" is a modifier on ghi, dni, etc.

echedey-ls commented 3 months ago

Maybe the lack of experience or the language barrier, but personally I find spectrum or spectra stand-alone reasonable. In the context of PV I doubt there is a spectrum other than of irradiance, and I've always (again, inexperience disclaimer) seen it as $W/m^2/[\mu , n]m$ so it's pretty normalized I think.

Perhaps "_spectral" is a modifier on ghi, dni, etc.

I don't dislike that possibility either, but I don't want to clog a name.

Regarding spectral_irrad, usually it is pythonic to write full words. So if you pursue that path, I recommend spectral_irradiance

RDaxini commented 3 months ago

Any irradiance quantity could have a corresponding spectral variable.

I think the plane and nature of the measurement matter. Perhaps "_spectral" is a modifier on ghi, dni, etc.

Good point, I hadn't considered it in that way. However, this formulation might not be as useful at this time since we do not currently have any functions that require the user to specify individual components of spectral irradiance. We do, however, have functions that operate on any spectral irradiance component as an input, so we still need some general variable to represent any spectral irradiance data (eg function 1 and 3 in the additional context of original post)

Regarding spectral_irrad, usually it is pythonic to write full words. So if you pursue that path, I recommend spectral_irradiance

Fair enough. Feels good as a function argument for the user but a bit long for writing out calculations (but maybe I am just lazy when I write code)

si / spectral_irradiance remove the singular/plural aspect, unlike spectrum/spectra, but I guess that's not a major issue. I think I could be on board with spectra if not spectral_irr or si

cwhanse commented 3 months ago

+1 to spectrum for a general variable (not specific to plane or aperture of the measurement). Spectrum "is the intensity of light as it varies with wavelength or frequency." (Brittannica). So a value of spectrum is a vector of ordered pairs (wavelength/frequency, intensity), or a function F intensity=F(wavelength) spectra would be more than one "value". We should give some thought as to preferred units, and how to represent the pair of vectors (or vector of pairs) of values.

I still favor adding ghi_spectral to the dictionary but after a need arises.

RDaxini commented 3 months ago

+1 to spectrum for a general variable (not specific to plane or aperture of the measurement)

Okay based on the discussion so far this sounds reasonable. One question: could there be any conflict due to the package being called spectrum as well?

I still favor adding ghi_spectral to the dictionary but after a need arises.

Agreed, I do too (you are ahead of your time Cliff!)

We should give some thought as to preferred units, and how to represent the pair of vectors (or vector of pairs) of values.

Current functions in pvlib.spectrum handle wavelength in nm, so I lean towards Wm^-2nm^-1 as the units of spectral irradiance. I think this is common outside of pvlib too but by no means universal, e.g. (IIRC) NSRDB FARMS-NIT simulations express wavelength in um. If I understood your point about representation correctly, I think it would be okay to stick with the current pvlib convention for now (wavelength index for a single pandas.Series spectrum, wavelength column headers for multiple spectra in a pandas.DataFrame)

RDaxini commented 3 months ago

@kandersolar you might be interested in this discussion

RDaxini commented 2 months ago

One question: could there be any conflict due to the package being called spectrum as well?

Since I now see spectrum as a variable does not work with from pvlib import spectrum, which is used in all pvlib spectrum tests (and I for one find very convenient to use in my own work) I am not in favour of spectrum as a variable. I think spectra, si, or even spectral_irr / spectral_irradiance would be better.

I have left spectrum in #2140 (but not the tests) as that currently has the most votes but perhaps this issue could be left open for a bit longer to see if anyone else has any thoughts before concluding/closing and adding something to the user guide variables page.

cwhanse commented 2 months ago

Since the function is designed to work with spectra (multiple spectrum) as input, I yield, spectra can be the variable.

RDaxini commented 2 months ago

Thoughts on the second and third bullet points in the original post?

  • Add the definition to the user guide
  • Update existing code if required

Is a user guide variable entry necessary, and should existing functions/tests be revised to all use the same spectra (Wm⁻²nm⁻¹) variable?

cwhanse commented 2 months ago

Is a user guide variable entry necessary

It is helpful but not necessary

should existing functions/tests be revised

The different input parameter names in spectrum.calc_spectral_mismatch_field seem harmless to me. I'm more concerned that the spectrum.spectrl2 output parameter is named spectra. This parameter is a dict of arrays. A user may expect that the output of spectrl2 (clear-sky spectral irradiance) should "fit" into the input of e.g. average_photon_energy.

RDaxini commented 2 months ago

It is helpful but not necessary

I am now stuck in a loop of wanting to be helpful but not be unnecessary

I'm more concerned that the spectrum.spectrl2 output parameter is named spectra. This parameter is a dict of arrays. A user may expect that the output of spectrl2 (clear-sky spectral irradiance) should "fit" into the input of e.g. average_photon_energy.

Ah good observation, I had not noticed that. Since the input for both spectrum.calc_spectral_mismatch_field and spectrum.average_photon_energy is pd.Series or pd.DataFrame, rather than changing the inputs there would the appropriate remedy be to modify the output of spectrum.spectrl2 to be a dataframe of spectra? I'd be happy to work on this. It would be good to have cross-compatibility, especially since now the variable (for at least two) is the same.

cwhanse commented 2 months ago

modify the output of spectrum.spectrl2 to be a dataframe of spectra?

I don't think we want to do that. There are multiple arrays in the output of spectrl2, for the different irradiance quantities, and each array can be a 2D array, with the columns corresponding to timestamps. Bundling the different irradiance quantities into a single Dataframe makes that Dataframe 3D, and that just feels awkward.

The simplest change would be to remove the word spectra from the docstring of spectrl2, i.e.,

Returns
--------
    dict
RDaxini commented 2 months ago

I don't think we want to do that. There are multiple arrays in the output of spectrl2, for the different irradiance quantities, and each array can be a 2D array, with the columns corresponding to timestamps. Bundling the different irradiance quantities into a single Dataframe makes that Dataframe 3D, and that just feels awkward.

Ah of course, you are right

The simplest change would be to remove the word spectra from the docstring of spectrl2

2168


I will have a think about modifying / how to modify average_photon_energy to accept more input types at a later date.