Calculate the effect of clouds ("cloud opacity factor") on spectral irradiance (spectrl2) #1201
Conversation
There was a problem hiding this comment.
Thanks for opening this PR @trondkr, and sorry for the delayed review! This seems like a good place to start for adding non-clear-sky spectral modeling to pvlib. I focused just on the new function in irradiance.py here and will do another review for the example later.
The new function in irradiance.py will need some tests. How should we create cases of inputs and expected outputs? Maybe use that pvlighthouse tool?
| `pvlib.irradiance.campbell_norman` irradiance with | ||
| clouds (transmittance) | ||
|
|
||
| irr_ghi_clouds:np.ndarray |
There was a problem hiding this comment.
I think these three parameters (according to [1]) are horizontal components, not in-plane components. It would be good to use the same parameter names as other functions (i.e. ghi and dhi), but I'm not sure pvlib has a name for direct horizontal irradiance -- perhaps dni_cos_zen or dni_projection?
There was a problem hiding this comment.
Also is np.ndarray a requirement? If scalars and pd.Series also work, then better to use the catch-all term numeric for the type.
| irr_ghi_clouds:np.ndarray | ||
| Total direct irradiance (poa_global) estimated | ||
| using `pvlib.irradiance.get_total_irradiance` and | ||
| `pvlib.irradiance.campbell_norman irradiance |
There was a problem hiding this comment.
Is campbell-norman actually a requirement? [1] seems to assume the user has measured irradiance components, so maybe the text for these parameters should be as simple as this: https://github.com/pvlib/pvlib-python/blob/master/pvlib/irradiance.py#L823-L830
|
|
||
| spectra:np.ndarray | ||
| Spectral irradiance output from `pvlib.spectrum.spectrl2` | ||
| under clear-sky conditions |
There was a problem hiding this comment.
- Similar to above, I'm not sure that this input needs a recommended source (note that [1] does not use spectrl2).
- I think the output of
spectrl2is actually a dict, not anndarray. But would it make sense to take the spectral inputs as individual parameters (i.e. have the user pass inspectra['poa_sky_diffuse'][:, 0]etc instead ofspectra)? Although pvlib often returns many outputs bundled together for convenience, inputs are usually kept separate.
| def to_datetimeindex(x): return x # noqa: E306 | ||
|
|
||
| def to_datetimeindex(x): | ||
| return x # noqa: E306 |
There was a problem hiding this comment.
Unfortunately the unrelated formatting changes here and below probably aren't desirable. Would it be easy to undo these changes, either through git revert, or maybe just by manually pasting over the text with the original version at https://github.com/pvlib/pvlib-python/blob/master/pvlib/irradiance.py?
I think the automated stickler-ci check will only complain about the formatting of code that is different from pvlib/master, so if the stuff below here is reverted back to the original, stickler shouldn't complain about it (even if your local linter does).
|
|
||
| First we calculate the rho fraction based on campbell_norman | ||
| irradiance with clouds converted to POA irradiance. In the | ||
| paper [1] these values are obtained from observations. The equations |
There was a problem hiding this comment.
| paper [1] these values are obtained from observations. The equations | |
| paper [1]_ these values are obtained from observations. The equations |
Adding an underscore like this turns the [1] into a clickable link to the reference text below
| def cloud_opacity_factor(irr_dif_clouds: np.ndarray, | ||
| irr_dir_clouds: np.ndarray, | ||
| irr_ghi_clouds: np.ndarray, | ||
| spectra: dict) -> (np.ndarray, np.ndarray): |
There was a problem hiding this comment.
Although we might add type hints eventually (see e.g. #1146 (comment)), I think for now better to keep the signature unannotated (and see below points about types anyway).
Edit: also we should think about the function name -- seems like the function name should have something to do with spectrum.
| wl = spectra['wavelength'] | ||
| irr_diff_s = np.trapz(y=spectra['poa_sky_diffuse'][:, 0], x=wl) | ||
| irr_dir_s = np.trapz(y=spectra['poa_direct'][:, 0], x=wl) | ||
| irr_glob_s = np.trapz(y=spectra['poa_global'][:, 0], x=wl) |
There was a problem hiding this comment.
Indexing with [:, 0] seems incorrect as it ignores every spectrum except the first. Shouldn't all input spectra be considered, or am I thinking about this incorrectly?
Or maybe I'm confused because I assumed that the function is supposed to work on time series inputs but is actually intended to only work on one spectrum at a time?
docs/sphinx/source/api.rstfor API changes.docs/sphinx/source/whatsnewfor 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`).This pull request adds a new function to irradiance.py which allows for the effect of cloud opacity to be accounted for when calculating spectral irradiance using spectrl2. I have added an example for how to use this to calculate the effects of clouds on irradiance.