8000 Spectral mismatch calculation code by adriesse · Pull Request #1524 · pvlib/pvlib-python · GitHub
[go: up one dir, main page]
Skip to content

Spectral mismatch calculation code #1524

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.

Sign up for GitHub

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

Merged
merged 19 commits into from
Sep 13, 2022
Merged

Spectral mismatch calculation code #1524

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
bb9b7be
Add mismatch module in pvlib.spectrum
adriesse Aug 13, 2022
949084a
Add utility function to produce a sample sr to work or play with.
adriesse Aug 13, 2022
12e06df
Add reference spectrum file and reader function.
adriesse Aug 15, 2022
70aa9ab
Add the mismatch calculation.
adriesse Aug 15, 2022
770480f
Add tests and expand doc strings.
adriesse Aug 16, 2022
ccd04fe
Update whatnew and placate Stickler.
adriesse Aug 16, 2022
9ab0e4c
Change am15 file from xls to csv; doc string expansion and misc small…
adriesse Aug 17, 2022
d9dd093
Adjust __init__.py and imports; sphynx references; whatsnew.
adriesse Aug 19, 2022
01ce237
Undo whatsnew v0.9.2
adriesse Aug 28, 2022
554fe07
Merge branch 'master' into mismatch
adriesse Aug 29, 2022
eab1afa
Merge branch 'pvlib:master' into mismatch
adriesse Aug 31, 2022
28c2eec
Resolve multiple issues from ongoing review.
adriesse Sep 2, 2022
35276c2
Make many improvements based on reviews.
adriesse Sep 3, 2022
0312e6d
Fix indentation and __init__.py
adriesse Sep 3, 2022
5cdb0fa
Polish docstrings some more.
adriesse Sep 3, 2022
db27486
Add explanation of truncation strategy.
adriesse Sep 3, 2022
42d369e
Final touches.
adriesse Sep 12, 2022
270d2ba
Expand explanation for truncation
adriesse Sep 12, 2022
80f9ea8
Merge branch 'master' into mismatch
cwhanse Sep 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Update whatnew and placate Stickler.
  • Loading branch information
@adriesse
adriesse committed Aug 16, 2022
commit ccd04fefad04489d040d68f48dd50631a86caca0
6 changes: 5 additions & 1 deletion docs/sphinx/source/whatsnew/v0.9.2.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ Enhancements
* Improve error message about uneven time intervals for
:py:func:`~pvlib.clearsky.detect_clearsky` and :py:func:`~pvlib.temperature.prilliman`
(:issue:`1476`, :pull:`1490`)
* New module and function to calculate spectral mismatch
:py:func:`~pvlib.spetrum.mismatch.calc_spectral_mismatch`
(:issue:`1523`, :pull:`1524`)

Bug fixes
~~~~~~~~~
Expand Down Expand Up @@ -59,9 +62,10 @@ Contributors
* Adam R. Jensen (:ghuser:`AdamRJensen`)
* Naman Priyadarshi (:ghuser:`Naman-Priyadarshi`)
* Chencheng Luo (:ghuser:`roger-lcc`)
* Prajwal Borkar (:ghuser:`PrajwalBorkar`)
* Prajwal Borkar (:ghuser:`PrajwalBorkar`)
* Kevin Anderson (:ghuser:`kanderso-nrel`)
* Cliff Hansen (:ghuser:`cwhanse`)
* Jules Chéron (:ghuser:`jules-ch`)
* Kurt Rhee (:ghuser:`kurt-rhee`)
* Will Hobbs (:ghuser:`williamhobbs`)
* Anton Driesse (:ghuser:`adriesse`)
6 changes: 5 additions & 1 deletion pvlib/spectrum/mismatch.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from scipy.interpolate import interp1d
import os


def get_sample_sr(wavelength=None):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to future-proof this function such that it will be able to return other spectral responses in the future? For example:

get_spectral_response(kind='c-si', wavelength=None)

^I would also prefer to write out "sr" as it is not immediately clear to me that it stands for "spectral response"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that calling out the material is important. For the name, I'd prefer something like get_example_sr or get_example_spectral_response, because I think sample could be more easily confused with a test device, as opposed to a reference device, and here it could be either.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed get_example_spectral_response would be good.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no problems making a name and parameter change to pass the review but I think it would be inappropriate to put additional technology options into this function in the future. I believe SR data should be in data files in general.

'''
Generate a generic smooth c-si spectral response for tests and experiments.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If data was found, could this function be extended to return spectral response for other cell types? It appears capable of doing so, and if that's correct, I'd suggest adding a kwarg cell_type="csi" or similar to switch between default data sets.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spectral response data should generally be in data files I think. (Lots coming.) This is really just something for people who have nothing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still vote for adding a kwarg now, rather than later.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you thinking of a NotImplemented exception for values other thane "csi" this time around?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like that, yes

Expand Down Expand Up @@ -56,6 +57,7 @@ def get_sample_sr(wavelength=None):

return sr


def get_am15g(wavelength=None):
'''
Read the ASTM G173-03 AM1.5 global tilted spectrum, optionally interpolated
Expand Down Expand Up @@ -100,6 +102,7 @@ def get_am15g(wavelength=None):

return am15g


def calc_spectral_mismatch(sr, e_sun, e_ref=None):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the name of this function should somehow reflect that it is specific to the case of a broadband reference device.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about: calc_spectral_mismatch_to_broadband()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I am going to update my suggestion to: calc_field_spectral_mismatch()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer calc_spectral_mismatch_field to make space for _lab, someday.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another idea: keep the function general (not specifically relative to broadband reference) add an optional sr_ref parameter that defaults to 1.0?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its a very reasonable suggestion, but perhaps it could be done in a future PR with some more debate about the range of interesting options and different ways of implementing them?

"""
Calculate the spectral mismatch under a given measured spectrum with
Expand Down Expand Up @@ -145,7 +148,8 @@ def calc_spectral_mismatch(sr, e_sun, e_ref=None):
sr_ref = np.interp(e_ref.T.index, sr.index, sr, left=0.0, right=0.0)

# a helper function to make usable fraction calculations more readable
integrate = lambda e: np.trapz(e, x=e.T.index, axis=-1)
def integrate(e):
return np.trapz(e, x=e.T.index, axis=-1)

# calculate usable fractions
uf_sun = integrate(e_sun * sr_sun) / integrate(e_sun)
Expand Down
6 changes: 3 additions & 3 deletions pvlib/tests/test_spectrum.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,8 +160,9 @@ def test_calc_mismatch(spectrl2_data):
assert_approx_equal(mm, 0.992393, significant=6)

# test with single sun spectrum, also used as reference spectrum
mm = mismatch.calc_spectral_mismatch(sr, e_sun=e_sun.loc['specglo'],
e_ref=e_sun.loc['specglo'])
mm = mismatch.calc_spectral_mismatch(sr,
e_sun=e_sun.loc['specglo'],
e_ref=e_sun.loc['specglo'])
assert_approx_equal(mm, 1.0, significant=6)

# test with multiple sun spectra
Expand All @@ -170,4 +171,3 @@ def test_calc_mismatch(spectrl2_data):
mm = mismatch.calc_spectral_mismatch(sr, e_sun=e_sun)
assert mm.index is e_sun.index
assert_allclose(mm, expected, rtol=1e-6)

0