-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 mismatch calculation code #1524
Conversation
Empty module: |
So we will probably have some interesting discussion on the type and shape of the outputs. While working on this I noticed that spectrl2 has wavelength as the first dimension. When converted to a dataframe this make wavelength the row index. I would prefer to have wavelength as the last dimension, which I find works more naturally. |
What is the envisioned scope of |
Yes, I think it would make some sense to move those two functions. At the moment I can't imagine |
I've been thinking about numpy friendliness here, and I am tending toward making these functions pandas only. The wavelength values are an integral part of the data so it makes a lot of sense to me to carry them around as row or column index. |
Ok, I think it works now. :) |
I think this is ready for review. Not sure whether I cause all the automated test failure--I hope not. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments while I'm working on wrapping my brain around this
pvlib/spectrum/mismatch.py
Outdated
|
||
def get_sample_sr(wavelength=None): | ||
''' | ||
Generate a generic smooth c-si spectral response for tests and experiments. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like that, yes
At least some of the failures are because |
Should I try attempt to change docs/sphinx/source/reference, or leave that to someone more familiar with this? |
Certainly not opposed to housing mismatch functions in I wonder if
Please do! I think adding a couple entries at the bottom of |
Ok, I made those adjustments. I'm afraid I don't understand why github still sees a conflict in the whatsnew though. |
pvlib/spectrum/mismatch.py
Outdated
|
||
def get_sample_sr(wavelength=None): | ||
''' | ||
Generate a generic smooth c-si spectral response for tests and experiments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like that, yes
Can someone suggest how to remove docs/sphinx/source/whatsnew/v0.9.2.rst from my PR? |
Using the example data for the spectral radiance of the sun and the spectral responsivity of a Si reference device, the error in the SMM when truncating both integrals appears to small, on the order of 0.1%. One does not, however, want to only truncate the incomplete sun integral.
Here is the code that produced these results: import numpy as np
from pvlib import spectrum
from pvlib.tests import test_spectrum
e_ref = spectrum.get_am15g() # 0.28 to 4 micron, scaled to "match" IEC 60904-3 Ed. 2 (2008)
e_ref_integral_to_1p7_micron = 942.88 # W/m^2, from IEC 60904-3 Ed. 2 (2008)
e_ref_integral_to_4_micron = 997.47 # W/m^2, from IEC 60904-3 Ed. 2 (2008)
e_ref_integral_to_infinity = 1000. # W/m^2, from IEC 60904-3 Ed. 2 (2008)
# Get example solar spectrum from test fixture.
_, e_sun = test_spectrum.spectrl2_data.__pytest_wrapped__.obj() # 0.3 to 4 micron
e_sun = e_sun.set_index('wavelength')
e_sun = e_sun.transpose().loc['specglo']
idx_to_1p7_micron = e_sun.T.index <= 1700
e_sun_integral_to_1p7_micron = np.trapz(e_sun[idx_to_1p7_micron], x=e_sun.T.index[idx_to_1p7_micron], axis=-1)
e_sun_integral_to_4_micron = np.trapz(e_sun, x=e_sun.T.index, axis=-1) # 708.4855034837434 W/m^2
scale_factor_estimate = e_ref_integral_to_infinity / e_ref_integral_to_4_micron
e_sun_integral_to_infinity = scale_factor_estimate * e_sun_integral_to_4_micron
sr = spectrum.get_example_spectral_response()
no_trunc = spectrum.calc_spectral_mismatch_field(sr, e_sun, e_sun_integral_to_infinity, e_ref=e_ref, e_ref_tot=e_ref_integral_to_infinity)
single_trunc_1p7_micron = spectrum.calc_spectral_mismatch_field(sr, e_sun[idx_to_1p7_micron], None, e_ref=e_ref, e_ref_tot=e_ref_integral_to_infinity)
single_trunc_4_micron_ = spectrum.calc_spectral_mismatch_field(sr, e_sun, None, e_ref=e_ref, e_ref_tot=e_ref_integral_to_infinity)
double_trunc_1p7_micron = spectrum.calc_spectral_mismatch_field(sr, e_sun[idx_to_1p7_micron], None)
double_trunc_4_micron = spectrum.calc_spectral_mismatch_field(sr, e_sun, None)
print("e_ref totals to 1.7 um, 4 um, and infinity (W/m^2):", e_ref_integral_to_1p7_micron, e_ref_integral_to_4_micron, e_ref_integral_to_infinity)
print("e_sun totals to 1.7 um, 4 um, and infinity (W/m^2):", e_sun_integral_to_1p7_micron, e_sun_integral_to_4_micron, e_sun_integral_to_infinity)
print("no_trunc smm:", no_trunc)
print("1.7 um single truncation smm:", single_trunc_1p7_micron)
print("4 um single truncation smm:", single_trunc_4_micron_)
print("1.7 um double truncation smm:", double_trunc_1p7_micron)
print("4 um double truncation smm:", double_trunc_4_micron)
print("1.7 um single truncation error in smm:", 100*(single_trunc_1p7_micron / no_trunc - 1), "%")
print("4 um single truncation error in smm:", 100*(single_trunc_4_micron_ / no_trunc - 1), "%")
print("1.7 um double truncation error in smm:", 100*(double_trunc_1p7_micron / no_trunc - 1), "%")
print("4 um double truncation error in smm:", 100*(double_trunc_4_micron / no_trunc - 1), "%") I had to change the source code in this PR in order to calculate the "no_trunc" result (doc strings omitted): def get_am15g(wavelength=None):
# Contributed by Anton Driesse (@adriesse), PV Performance Labs. Aug. 2022
# Changed by Mark Campanelli for testing purposes.
pvlib_path = pvlib.__path__[0]
filepath = os.path.join(pvlib_path, 'data', 'astm_g173_am15g.csv')
am15g = pd.read_csv(filepath, index_col=0).squeeze() # 0.28 to 4 microns
am15g_integral_to_4_micron = np.trapz(am15g, x=am15g.index, axis=-1) # W/m^2
iec_integral_to_4_micron = 997.47 # W/m^2, from IEC 60904-3 Ed. 2 (2008)
am15g_scaled_to_iec = iec_integral_to_4_micron / am15g_integral_to_4_micron * am15g
if wavelength is not None:
interpolator = interp1d(am15g_scaled_to_iec.index, am15g_scaled_to_iec,
kind='linear',
bounds_error=False,
fill_value=0.0,
copy=False,
assume_sorted=True)
am15g_scaled_to_iec = pd.Series(data=interpolator(wavelength), index=wavelength)
am15g_scaled_to_iec.index.name = 'wavelength'
am15g_scaled_to_iec.name = 'am15g'
return am15g_scaled_to_iec
def calc_spectral_mismatch_field(sr, e_sun, e_sun_tot, e_ref=None, e_ref_tot=None):
# Contributed by Anton Driesse (@adriesse), PV Performance Labs. Aug. 2022.
# Changed by Mark Campanelli for testing purposes.
# get the reference spectrum at wavelengths matching the measured spectra
if e_ref is None:
e_ref = get_am15g(wavelength=e_sun.T.index)
# a helper function to make usable fraction calculations more readable
def integrate(e):
return np.trapz(e, x=e.T.index, axis=-1)
if e_sun_tot is None:
# Use integrated spectral irradiance, typically an approximation
# that misses "tail" of distribution.
e_sun_tot = integrate(e_sun)
if e_ref_tot is None:
# Use integrated spectral irradiance, typically an approximation
# that misses "tail" of distribution.
e_ref_tot = integrate(e_ref)
# interpolate the sr at the wavelengths of the spectra
# reference spectrum wavelengths may differ if e_ref is from caller
sr_sun = np.interp(e_sun.T.index, sr.index, sr, left=0.0, right=0.0)
sr_ref = np.interp(e_ref.T.index, sr.index, sr, left=0.0, right=0.0)
# calculate usable fractions
uf_sun = integrate(e_sun * sr_sun) / e_sun_tot
uf_ref = integrate(e_ref * sr_ref) / e_ref_tot
# mismatch is the ratio or quotient of the usable fractions
smm = uf_sun / uf_ref
if isinstance(e_sun, pd.DataFrame):
smm = pd.Series(smm, index=e_sun.index)
return smm I am guessing that double truncation is not a bad strategy in many (most?) cases, but I will point out that an alternate strategy (ref. Carl Osterwald) is to estimate the total sun spectrum by integrating the tail of the reference spectrum after scaling it to "match" some portion of the sun spectrum. I suppose we would have to do a bigger study to fairly compare the two approaches. I have some additional thoughts now about how we could (optionally) include the total irradiance value(s) but still try to keep the function interface from being too confusing. (This idea is different than the code changes I used above.) That said, I would also be ok if the double truncation approximation was more clearly stated in the docstring and perhaps a note saying that the code is not appropriate for computations such as primary reference cell calibrations such as ASTM E1125. (It turns out that ASTM E1125 is where the differences between ASTM G173-03 and IEC 60904-3 is called out and handled.) |
Thanks very much, @markcampanelli for checking this out so thoroughly and confirming at the very least that I'm not terribly mistaken! :) The double truncation is more or less imposed when one of the inputs is pre-truncated in field measurements. One additional easy test is to image the sun spectrum being equal to the reference spectrum, so you want to get a factor of 1.0. If the spectroradiometer only measures to 1700 nm, then the only way to get 1.0 is to truncate the reference spectrum to 1700 as well. I've added a paragraph to the docstring to explain the truncation. |
Photovoltaic Reference Cell" :doi:`10.1520/E0973-10` | ||
.. [3] IEC 60904-7 "Computation of the spectral mismatch correction | ||
for measurements of photovoltaic devices" | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about IEC 61853-3 (ed. 1.0), specifically, equation (6) in section 7.3?
This is where the implementation here does not cover that formula precisely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate a little bit on where you see the problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In any case, the equation shows the pre-integrated values of the reference and sun/field spectra in a similar manner to your proposed function signature. Do the pre-integrated values need to be different from the values that would be obtained from an actual integration?
I have some comments about the problems with this calculation of the standard in the docstring of my implementation in pvpltools.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adriesse Sorry that I have not been able to look at this closely again this week.
Do the pre-integrated values need to be different from the values that would be obtained from an actual integration?
This is perhaps the key question, assuming that the IEC 61853-3 use case is an important one for this code addition. (I have some issues with this formula, but it is what it is.) At this point I am assuming that this use case is important, and it requires some consideration in addition to the "primary reference cell calibration with a cavity radiometer" use case that I previously examined here. I intend to look closer at this this weekend.
There is also one last use case, which I am personally interested in. This is for PVfit's "irradiance" calculation of F=Isc/Isc0
. The spectral-mismatch approach here provides an alternative to the approach that I presented at the recent PVPMC workshop. While I would like to see this code cover this case to my satisfaction, please know that I am not trying to hold up this PR based on this criterion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might prefer to see future functions for specific use cases such as calibration or energy rating. A single generic function could be fewer lines of code, but may not be as inviting for users who are not as broadly informed on the subject.
I have been involved in many discussions about IEC 61853-3. The next version will look different, probably skipping the mismatch or spectral correction factor in this form altogether. Anyway, it would be hard to cover the current version in this function due to the use of banded solar spectral irradiance data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I derived equation (6) in section 7.3 in IEC 61853-3 (ed. 1.0) from first principles. Recall that this is used in IEC 61853-3 to compute the equivalent irradiance under the standard spectrum that would generate the same short-circuit current in the device under the given operating condition, denoted here as E_equiv
. One issue I found is that technically the spectral response of the device should depend on temperature. A second issue to note is that the upper limit of integration on the integrals, 'lambda_e", is fixed at 4 microns, which corresponds to the assumed spectral-response cutoff of total POA irradiance as measured by a pyranometer and (presumably) reported in the profiles in IEC 61853-4. In the study below, a cutoff change to 1.7 microns requires scaling the total POA irradiance accordingly.
I then computed the SSM and E_equiv
that would be computed by code under review vs. an implementation that covers equation (6) in IEC 61853-3. The error appears small for the 4 micron cutoff, but becomes very significant for the 1.7 micron cutoff. What is quite interesting, and what I did not expect, is that the SMM changes significantly based on the wavelength cutoff, and this change is necessary to compute a consistent E_equiv
! Of course, these results are contingent upon 3rd-party review :).
This is the modified source code to run the comparison. It can be used to compute both algorithms.
def get_am15g(wavelength=None):
# Contributed by Anton Driesse (@adriesse), PV Performance Labs. Aug. 2022
# Modified by Mark Campanelli for testing purposes.
pvlib_path = pvlib.__path__[0]
filepath = os.path.join(pvlib_path, 'data', 'astm_g173_am15g.csv')
am15g = pd.read_csv(filepath, index_col=0).squeeze() # 0.28 to 4 microns
am15g_integral_to_4_micron = np.trapz(am15g, x=am15g.index, axis=-1) # W/m^2
iec_integral_to_4_micron = 997.47 # W/m^2, from IEC 60904-3 Ed. 2 (2008)
am15g_scaled_to_iec = iec_integral_to_4_micron / am15g_integral_to_4_micron * am15g
if wavelength is not None:
interpolator = interp1d(am15g.index, am15g,
kind='linear',
bounds_error=False,
fill_value=0.0,
copy=False,
assume_sorted=True)
am15g = pd.Series(data=interpolator(wavelength), index=wavelength)
interpolator = interp1d(am15g_scaled_to_iec.index, am15g_scaled_to_iec,
kind='linear',
bounds_error=False,
fill_value=0.0,
copy=False,
assume_sorted=True)
am15g_scaled_to_iec = pd.Series(data=interpolator(wavelength), index=wavelength)
am15g.index.name = 'wavelength'
am15g.name = 'am15g'
am15g_scaled_to_iec.index.name = 'wavelength'
am15g_scaled_to_iec.name = 'am15g'
return am15g, am15g_scaled_to_iec
def calc_spectral_mismatch_field(sr, e_sun, e_sun_tot, e_ref=None, e_ref_tot=None):
# Contributed by Anton Driesse (@adriesse), PV Performance Labs. Aug. 2022
# Modified by Mark Campanelli for testing purposes.
# get the reference spectrum at wavelengths matching the measured spectra
if e_ref is None:
e_ref, _ = get_am15g(wavelength=e_sun.T.index)
# a helper function to make usable fraction calculations more readable
def integrate(e):
return np.trapz(e, x=e.T.index, axis=-1)
if e_sun_tot is None:
# Use integrated spectral irradiance, typically an approximation
# that misses "tail" of distribution.
e_sun_tot = integrate(e_sun)
if e_ref_tot is None:
# Use integrated spectral irradiance, typically an approximation
# that misses "tail" of distribution.
e_ref_tot = integrate(e_ref)
# interpolate the sr at the wavelengths of the spectra
# reference spectrum wavelengths may differ if e_ref is from caller
sr_sun = np.interp(e_sun.T.index, sr.index, sr, left=0.0, right=0.0)
sr_ref = np.interp(e_ref.T.index, sr.index, sr, left=0.0, right=0.0)
# calculate usable fractions
uf_sun = integrate(e_sun * sr_sun) / e_sun_tot
uf_ref = integrate(e_ref * sr_ref) / e_ref_tot
# mismatch is the ratio or quotient of the usable fractions
smm = uf_sun / uf_ref
if isinstance(e_sun, pd.DataFrame):
smm = pd.Series(smm, index=e_sun.index)
return smm
This is the Jupyter notebook code that runs the comparison:
import numpy as np
from pvlib import spectrum
from pvlib.tests import test_spectrum
# Standard spectra from 0.28 to 4 micron, e_ref_iec is am15g (from ASTM G173-03) scaled to "match" IEC 60904-3 Ed. 2 (2008).
e_ref_am15g_astm, e_ref_am15g_iec = spectrum.get_am15g()
# IEC 60904-3 Ed. 2 (2008), cumulatives are provided in standard.
e_ref_am15g_iec_idx_to_1p7_micron = e_ref_am15g_iec.T.index <= 1700
e_ref_am15g_iec_integral_to_1p7_micron = 942.88 # W/m^2
e_ref_am15g_iec_integral_to_4_micron = 997.47 # W/m^2
e_ref_am15g_iec_integral_to_infinity = 1000. # W/m^2
# ASTM G173-03, calculate cumulatives not provided in standard.
e_ref_am15g_astm_idx_to_1p7_micron = e_ref_am15g_astm.T.index <= 1700
e_ref_am15g_astm_integral_to_1p7_micron = np.trapz(e_ref_am15g_astm[e_ref_am15g_astm_idx_to_1p7_micron], x=e_ref_am15g_astm.index[e_ref_am15g_astm_idx_to_1p7_micron], axis=-1) # W/m^2
e_ref_am15g_astm_integral_to_4_micron = np.trapz(e_ref_am15g_astm, x=e_ref_am15g_astm.index, axis=-1) # W/m^2
e_ref_am15g_astm_integral_to_infinity = None # W/m^2, undefined in standard
# Get example solar spectrum from test fixture.
_, e_sun = test_spectrum.spectrl2_data.__pytest_wrapped__.obj() # 0.3 to 4 micron
e_sun = e_sun.set_index('wavelength')
e_sun = e_sun.transpose().loc['specglo']
e_sun_idx_to_1p7_micron = e_sun.T.index <= 1700
e_sun_integral_to_1p7_micron = np.trapz(e_sun[e_sun_idx_to_1p7_micron], x=e_sun.T.index[e_sun_idx_to_1p7_micron], axis=-1)
e_sun_integral_to_4_micron = np.trapz(e_sun, x=e_sun.T.index, axis=-1) # 708.4855034837434 W/m^2
scale_factor_estimate = e_ref_am15g_iec_integral_to_infinity / e_ref_am15g_iec_integral_to_4_micron
e_sun_integral_to_infinity = scale_factor_estimate * e_sun_integral_to_4_micron
# Relative spectral response for Si, 0.29 to 1.19 micron.
sr = spectrum.get_example_spectral_response()
# SMM computations IEC 61853-3 imply reference temperature in all situations, which is technically incorrect.
# Calculate SMM using formula in IEC 61853-3, using IEC 60904-3 Ed. 2 (2008) spectrum.
# Different cutoff wavelengths used for solar total irradiance integral according to the spectraradiometer cutoff.
iec_smm_inf = spectrum.calc_spectral_mismatch_field(
sr, e_sun, e_sun_integral_to_infinity, e_ref=e_ref_am15g_iec, e_ref_tot=e_ref_am15g_iec_integral_to_infinity)
iec_smm_4_micron = spectrum.calc_spectral_mismatch_field(
sr, e_sun, e_sun_integral_to_4_micron, e_ref=e_ref_am15g_iec, e_ref_tot=e_ref_am15g_iec_integral_to_infinity)
iec_smm_1p7_micron = spectrum.calc_spectral_mismatch_field(
sr, e_sun[e_sun_idx_to_1p7_micron], e_sun_integral_to_1p7_micron, e_ref=e_ref_am15g_iec, e_ref_tot=e_ref_am15g_iec_integral_to_infinity)
# Calculate SMM using proposed formula double truncation of integrals with ASTM G173-03 spectrum.
astm_smm_4_micron = spectrum.calc_spectral_mismatch_field(sr, e_sun, None)
astm_smm_1p7_micron = spectrum.calc_spectral_mismatch_field(sr, e_sun[e_sun_idx_to_1p7_micron], None)
print("IEC am15g totals to 1.7 um, 4 um, and infinity (W/m^2):", e_ref_am15g_iec_integral_to_1p7_micron, e_ref_am15g_iec_integral_to_4_micron, e_ref_am15g_iec_integral_to_infinity)
print("ASTM am15g totals to 1.7 um, 4 um, and infinity (W/m^2):", e_ref_am15g_astm_integral_to_1p7_micron, e_ref_am15g_astm_integral_to_4_micron, e_ref_am15g_astm_integral_to_infinity)
print("e_sun totals to 1.7 um, 4 um, and infinity (W/m^2):", e_sun_integral_to_1p7_micron, e_sun_integral_to_4_micron, e_sun_integral_to_infinity)
print("IEC smm to infinity:", iec_smm_inf)
print("IEC smm to 4 um:", iec_smm_4_micron)
print("IEC smm to 1.7 um:", iec_smm_1p7_micron)
print("IEC am15g-equivalent E to infinity:", iec_smm_inf*e_sun_integral_to_infinity)
print("IEC am15g-equivalent E to 4 um:", iec_smm_4_micron*e_sun_integral_to_4_micron)
print("IEC am15g-equivalent E to 1.7 um:", iec_smm_1p7_micron*e_sun_integral_to_1p7_micron)
print("ASTM smm to 4 um:", astm_smm_4_micron)
print("ASTM smm to 1.7 um:", astm_smm_1p7_micron)
print("ASTM am15g-equivalent E to 4 um:", astm_smm_4_micron*e_sun_integral_to_4_micron)
print("ASTM am15g-equivalent E to 1.7 um:", astm_smm_1p7_micron*e_sun_integral_to_1p7_micron)
Here is the output for the comparison. Note the consistency of E_equiv
in the "IEC version" of the computation.
IEC am15g totals to 1.7 um, 4 um, and infinity (W/m^2): 942.88 997.47 1000.0
ASTM am15g totals to 1.7 um, 4 um, and infinity (W/m^2): 945.6188921247738 1000.3706555734423 None
e_sun totals to 1.7 um, 4 um, and infinity (W/m^2): 668.028852782929 708.4855034837434 710.2825182549283
IEC smm to infinity: 0.9913868433999364
IEC smm to 4 um: 0.9939014139772988
IEC smm to 1.7 um: 1.054093308636961
IEC am15g-equivalent E to infinity: 704.1647436949111
IEC am15g-equivalent E to 4 um: 704.1647436949111
IEC am15g-equivalent E to 1.7 um: 704.164743694911
ASTM smm to 4 um: 0.9923965546943565
ASTM smm to 1.7 um: 0.9902678450884945
ASTM am15g-equivalent E to 4 um: 703.0985727081635
ASTM am15g-equivalent E to 1.7 um: 661.5274925022902
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for your detailed analysis, Mark. I agree that there is potential for small differences with the tails, and large differences due to the cut-off at 1700 nm. In this field application the cutoff at 1700 nm is only made if there is no information available about the spectrum beyond. In that situation there are not a lot of choices available, and cutting off the reference spectrum at the same wavelength appears to be a reasonable solution.
@nicorie would you be interested in having a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice PR @adriesse! One minor change and one idea to consider below.
pvlib/spectrum/mismatch.py
Outdated
return am15g | ||
|
||
|
||
def calc_spectral_mismatch(sr, e_sun, e_ref=None): |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments that can be considered, but looks good :)
To all: if you feel that this PR is premature, don't feel like you have to merge it. It's not a lot of code and I can easily just pack it into an example notebook. |
Well this PR does have three approving reviews, which is more than most pvlib PRs :) I haven't been following the discussion with @markcampanelli closely, but I have the sense that at this point whatever limitations there are in this implementation are noted in the docstring, and of course we can always make additional improvements later if we want. Anyone please correct me if I'm mistaken there. Otherwise let's plan to merge this tomorrow? |
@adriesse Would it be possible to include an example in this PR of how this code is used for a specific use case or cases? If I were to try to use it for an IEC 61853-3 like calculation with a 1.7 micron cutoff, then I could get tripped up about which total irradiance to multiply the SMM by to get the equivalent irradiance under the standard spectrum used in the 61853-1 matrix (i.e., using the double truncation approximation). I am curious if |
@markcampanelli An example will accompany the forthcoming sr collection and spectroradiometer measurements on Duramat. |
There are conflicts with the main branch, maybe what’s new? That must be addressed before we can merge |
I think this needs coordination with the other PR(s), doesn't it? |
The conflict was created when #1468 was merged. I can resolve if you'd like the help. |
Sure @cwhanse, it seems easiest to do this at the time of merging. |
@markcampanelli are you OK with this being merged and opening issues for improvements? |
Yes. Note: I am running some computation comparisons with PVfit, and if I find anything to reconcile, then I will make a followup issue. |
Thanks @adriesse for this addition (which I myself plan to use) and @markcampanelli for the effort in ensuring pvlib continues to be a reliable and high-quality tool :) |
Thanks to all participants here as well!! There's definitely room for further work on mismatch calculation methods, both for pvlib and for scientific publications. |
For the example SR and reference and solar spectra provided herein, I did not find any significant numerical differences between this implementation and PVfit's |
Thanks a lot for checking that @markcampanelli ! |
I didn't use a strong enough word there: there a need for further work. |
docs/sphinx/source/reference
for API changes.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`
).remote-data
) and Milestone are assigned to the Pull Request and linked Issue.