add Boland DF estimation #1179
add Boland DF estimation #1179
Conversation
|
Hi @cwhanse, interested in reviewing the gallery example and added Boland DNI estimation function? Thanks |
- move boland enhancement what's new from v0.9.0 --> v0.9.5 - move boland sphinx doc entry from api.rst (deleted) to reference/irradiance/decomposition
- put Badescu textbook before paper - revise wording defining kt
- remove redundant kt definition - use intersphinx to link to pandas and numpy types
responding to comments, simplify wording L12-15 to omit "systems often tilted to optimize performance..." Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
respond to comments, reword decomposition example intro Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
- add intro before functions in example
- fix use :py:func: to xref functions in docs, instead of :meth: - add comment to explain conversion of mbars to Pa - use Gsc not E0 in kt comparison plot
- add section to conclusions to warn users that TMY3 and NSRDB are also models, therefore not to assume differences are errors, and link to TMY3 & NSRDB documentation - use implicit targets to link to DISC, DIRINT, Erbs, & Boland sections - reverse change of scaled GHI by E0 to Gsc (aka: solar constant) - use math directive for DNI formula
- revise & condense conclusions, don't refer to differences as errors without operational data - add header before plots section, explain why comparing normalized GHI - add subheaders for seasonal plots
|
Hi @cwhanse sorry for the very long delay in responding. I have accepted your changes, added discussions for your suggestions, and responded to your comments. Let me know if you have any additional feedback. Thanks! |
|
BTW: I think the readthedocs failure is unrelated, I'm trying to figure it out - the bifacial gallery examples are failing with this error:
IDK why but readthedocs is pulling multiple versions of pvfactors, finally settling on v1.4.1 instead of the latest v1.5.2 which limits shapely<v2. It must have something to do with readthedocs b/c it was building the docs a few commits ago, and nothing else has changed. Truly bizarre: [UPDATE]: RTD is getting the pvlib version wrong, that's why pvfactors downgrades causing the wrong shapely to install. See below |
|
|
|
@kanderso-nrel I've figured out why readthedocs is going bonkers. For some reason the version is being read as pvlib-0.9.0a5.dev58+g EG: the last commit (9715084): This only started happening after I merged with main 9d9421e, but oddly, that was the last commit that did have the correct version: pvlib-0.9.5.dev16+g9d9421e And readthedocs did build. This has something to do with the pep517/518 changes, When the version is misread as v0.9.0, that causes pvfactors to downgrade, and that causes shapely>v2 to be installed leading to the build failure. I don't understand how the new build system works, all I can hope is that it all works out when/if this gets merged. |
This reverts commit ccf40a8.
|
You can view a rendering of the docs here: https://pvlib-python--1179.org.readthedocs.build/en/1179/ I guess the readthedocs failures has something to do with from importlib_metadata import version
import pvlib
version('pvlib')
# '0.9.5.dev34+g1ae35cd'Before I purged the egg-info file, it was returning 0.9.0! |
Use Sphinx math role to render k_t in docstring for min cosine zenith Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
|
@pvlib/pvlib-maintainer any more comments on this? Someone care to do the honors? |
|
I'm out back country skiing but I'd like to review on Monday if it isn't merged by then 😎 |
| df = 1.0 / (1.0 + np.exp(-5.0 + 8.6 * kt)) | ||
| # NOTE: [1] has different coefficients, for different time intervals | ||
| # 15-min: df = 1 / (1 + exp(8.645 * (kt - 0.613))) | ||
| # 1-hour: df = 1 / (1 + exp(7.997 * (kt - 0.586))) |
There was a problem hiding this comment.
If there are different coefficient values of interest, should we expose them as optional parameters?
There was a problem hiding this comment.
For now, I recommend a kwarg coefficients and including these in the Notes section of the docstring. A follow up could add an option to infer the appropriate coefficients. Inference should definitely be in a function boland_diffuse_fraction. I'd like to see that function in this PR too, but I won't push it.
There was a problem hiding this comment.
I rearranged formula to match the reference, as well as the coefficients, which had been rounded to 8.6 and 5 (=5.3=8.645*0.613).
There was a problem hiding this comment.
I think if we're going to call it a boland "model", the coefficients are part of the model. If they are not, we can call it logistic function model and offer the boland "coefficients" to accompany it.
In my parallel universe I call it the boland model and offer a "period"option that can have values of 15 or 60.
And for all the fans of hourly data, that should probably be the default.
There was a problem hiding this comment.
I'd favor renaming to logistic and naming coefficient sets, if there was another author that fit this same equation. I'm not aware of any.
| # ensure that closure relationship remains valid | ||
| dhi = np.where(bad_values, ghi, dhi) | ||
|
|
||
| data = OrderedDict() |
There was a problem hiding this comment.
Is there any reason to continue using OrderedDict in new code now that we only support python 3.7 and up?
There was a problem hiding this comment.
TBH, IDK, but all of the functions in this module use this. I'll open a separate issue to respond to this. See #1684 .
| bad_values = (zenith > max_zenith) | (ghi < 0) | (dni < 0) | ||
| dni = np.where(bad_values, 0, dni) | ||
| # ensure that closure relationship remains valid | ||
| dhi = np.where(bad_values, ghi, dhi) |
There was a problem hiding this comment.
I know these check have been copied a few times (along with most of the function) and I told myself a few times I should look at them more closely one day... If ghi were clipped at zero and zenith constrained as advertised, could there still be any bad values?
- add a_coeff, b_coeff to docstring params - update equation to match Boland paper, use A, B coeffs - update coeffs to match Boland paper too (8.6-> 8.645 and 5 -> 5.3 - add note to explain different coeffs for different time intervals - give coeffs for 15-min & 1-hr - update zenith to solar_zenith everywhere - update test expected to match output with new coeffs
- create irradiance-decomposition subdir in gallery, add readme.rst - use complete_irradiance() to get DHI using closure eqn's
- in plot diffuse fraction examples - add note to explain b/c TMY timestampe at hour end - reset index to align with TMY3 - fix Jan, July, Apr timestamp names didn't match actual times
- chnage df to be specific to disc or dirint - change date names to Jan-04, Jan-07, etc. instead of _AM, _PM etc.
|
@kanderso-nrel thanks for tips. I think I've addressed everything. @AdamRJensen looking forward to your comments. |
|
For the examples, you could consider (1) a df vs. kt plot and (2) some predicted vs. measured scatter plots. |
There was a problem hiding this comment.
Great stuff @mikofski!
Only a few minor comments from my side.
docs/examples/irradiance-decomposition/plot_diffuse_fraction.py
Outdated
Show resolved
Hide resolved
| # of data measured from 1990 to 2010. Therefore we change the timestamps to a | ||
| # common year, 1990. | ||
| DATA_DIR = pathlib.Path(pvlib.__file__).parent / 'data' | ||
| greensboro, metadata = read_tmy3(DATA_DIR / '723170TYA.CSV', coerce_year=1990) |
There was a problem hiding this comment.
As you mention later, it's not ideal to compare against modeled data. I guess we do not currently have any measurement data files? Perhaps it is desirable to add an example measurement data set (in a follow-up issue)?
docs/examples/irradiance-decomposition/plot_diffuse_fraction.py
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,218 @@ | |||
| """ | |||
| Diffuse Fraction Estimation | |||
There was a problem hiding this comment.
Given that the section covers both decomposition models that estimate the diffuse and direct components, this title seems misleading.
| from pvlib.iotools import read_tmy3 | ||
| from pvlib.solarposition import get_solarposition | ||
| from pvlib import irradiance | ||
| import pvlib |
There was a problem hiding this comment.
I much prefer to only import pvlib once and not a mix of modules and functions (in this example four different lines are dedicated to pvlib imports).
My main reason for this is that as a user of a new python library, I often find myself wondering from which packages imported functions come from and have to scroll to the top of the script. By writing out the full path, e.g., pvlib.iotools.read_tmy3, we're also providing some context about the module structure in pvlib, which I think is useful for new users to get familiar with. From an initial screening, it doesn't seem to make the script much longer. Just a mild preference though.
| @@ -12,5 +12,6 @@ DNI estimation models | |||
| irradiance.dirint | |||
| irradiance.dirindex | |||
| irradiance.erbs | |||
| irradiance.boland | |||
There was a problem hiding this comment.
It's a bit odd that this section of the API documentation is titled "DNI Estimation models", when the Boland and Erbs models are a diffuse fraction estimation models. This is somewhat in line with my comment about the title of the gallery example. I think that this section should be titled "Decomposition models" in order to encompass both DNI and DHI estimation models. Decomposition models also seem to be a more commonly used term than DNI/DHI estimation models anyhow.
* use lower case coeffs in equation * periods in docstrings * some wordsmithing * replace parameter types for datetime_or_doy with numeric * consistent def for kt is ratio of GHI to horizontal extraterrestrial-irradiance Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
|
@pvlib/pvlib-maintainer ready to merge |
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`).pvlib.irradiance.bolandis another diffuse fraction, DF, estimation method similar to Erbs but uses a single logistic exponential correlation between DF and clearness index, kt, that is continuously differentiable and bounded between zero and one.