Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
add Boland DF estimation #1179
add Boland DF estimation #1179
Changes from 22 commits
c6116eb
fc9fad2
fc358b6
a69333b
2faf9ab
9d9421e
6c8cb94
21da189
cfee27f
9cc772a
9bdd1bb
cbb61bd
c9ce7b8
e23662d
90f4434
ccf40a8
b1027bc
28fab4f
9715084
1ae35cd
ecdb508
27ff16e
8e92381
9b7f6d8
dc75be3
bee5bfd
52ca13c
ef31c9a
1a3e280
20e9a53
258ffd2
1f6b939
e87cd57
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
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.
See discussion #1696
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 there are different coefficient values of interest, should we expose them as optional parameters?
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.
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 functionboland_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.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 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?
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.
see #1685
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH, IDK, but all of the functions in this module use this. I'll open a separate issue to respond to this. See #1684 .
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 favor of regular dict.