Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"spectral irradiance" variable defintion/standardisation suggestion #2150

Open
RDaxini opened this issue Aug 1, 2024 · 13 comments · Fixed by #2168
Open

"spectral irradiance" variable defintion/standardisation suggestion #2150

RDaxini opened this issue Aug 1, 2024 · 13 comments · Fixed by #2168

Comments

@RDaxini
Copy link
Contributor

RDaxini commented Aug 1, 2024

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

  • Agree a standard definition for spectral irradiance as a variable
    I think having a standard "root" variable that can have super/subscripts added to for function-specific requirements would be good, for example, something like ghi / ghi_extra, aoi / aoi_projection , or temp_air / temp_cell / temp_dew (the "root" here being ghi, aoi, and temp)

  • Add the definition to the user guide

  • Update existing code if required

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
Copy link
Member

cwhanse commented Aug 1, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

RDaxini commented Aug 2, 2024

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
Copy link
Member

cwhanse commented Aug 2, 2024

+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
Copy link
Contributor Author

RDaxini commented Aug 2, 2024

+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
Copy link
Contributor Author

RDaxini commented Aug 5, 2024

@kandersolar you might be interested in this discussion

@RDaxini
Copy link
Contributor Author

RDaxini commented Aug 13, 2024

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
Copy link
Member

cwhanse commented Aug 13, 2024

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

RDaxini added a commit to RDaxini/pvlib-python that referenced this issue Aug 14, 2024
"spectrum" variable --> "spectra" (see pvlib#2150)
RDaxini added a commit to RDaxini/pvlib-python that referenced this issue Aug 14, 2024
"spectrum" variable --> "spectra" (see pvlib#2150)
kandersolar added a commit that referenced this issue Aug 14, 2024
* create

* Update spectrum.rst

* Update test_spectrum.py

* Update mismatch.py

update docstring

* Update mismatch.py

* Update mismatch.py

fix docs typesetting

* Update mismatch.py

* docs, tests

* docs, data checks, tests

add checks in function for negative irradiance and invalid data type
add tests for these checks
update docs maths typesetting

* Update mismatch.py

still trying to typeset the units without using / / between multiple length units

* Update mismatch.py

trying unicode superscript

* Apply suggestions from code review

Co-authored-by: Kevin Anderson <[email protected]>

* remove comments, inverse fraction

* remove comment

* Update irradiance.py

* Update test_irradiance.py

* Update test_irradiance.py

* Update irradiance.py

update returns statement, change symbols, suppress runtimewarning for division by zero

Co-Authored-By: Kevin Anderson <[email protected]>

* mixed up my lambdas and gammas:(

* change variable name spectral_irr -> spectrum

* update variable name in docstring + error messages

* Update irradiance.py

* Update test_irradiance.py

add test for return np.nan in case of zero si input

* Apply suggestions from code review

Co-authored-by: Cliff Hansen <[email protected]>

* Update irradiance.py

* Update v0.11.1.rst

* Update irradiance.py

apple suggestions from code review

Co-Authored-By: Adam R. Jensen <[email protected]>

* Update irradiance.py

"spectrum" variable --> "spectra" (see #2150)

* Update test_irradiance.py

"spectrum" variable --> "spectra" (see #2150)

* Update test_mismatch.py

remove unintentional change

* Update v0.11.1.rst

apply suggestion from code review

Co-Authored-By: Kevin Anderson <[email protected]>

* Update irradiance.py

sentence correction

Co-Authored-By: Kevin Anderson <[email protected]>

* update output datatype and associated tests

return series in case of dataframe input, add test to verify. Remove unnecessary spaces from tests

* Update test_irradiance.py

update test for datatype according to suggestion from code review

* Update docs/sphinx/source/whatsnew/v0.11.1.rst

Co-authored-by: Kevin Anderson <[email protected]>

---------

Co-authored-by: Kevin Anderson <[email protected]>
Co-authored-by: Kevin Anderson <[email protected]>
Co-authored-by: Cliff Hansen <[email protected]>
Co-authored-by: Adam R. Jensen <[email protected]>
@RDaxini
Copy link
Contributor Author

RDaxini commented Aug 15, 2024

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
Copy link
Member

cwhanse commented Aug 15, 2024

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
Copy link
Contributor Author

RDaxini commented Aug 15, 2024

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
Copy link
Member

cwhanse commented Aug 15, 2024

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
Copy link
Contributor Author

RDaxini commented Aug 15, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants