Skip to content
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

Update irradiance.klucher docs #2192

Merged
merged 18 commits into from
Sep 26, 2024
Merged

Conversation

RDaxini
Copy link
Contributor

@RDaxini RDaxini commented Aug 30, 2024

Docs here

move eqn and add description and variables definition to a new notes section
update references
redefine surface tilt for consistency with pvlib#2191
@RDaxini RDaxini marked this pull request as ready for review August 30, 2024 11:59
@RDaxini RDaxini marked this pull request as draft August 30, 2024 12:02
update variable names (I_d0 to DHI and add definition of theta as aoi)
@kandersolar kandersolar changed the title Update irradiance.kulcher docs Update irradiance.klucher docs Aug 30, 2024
wording in definition of F'
@RDaxini RDaxini marked this pull request as ready for review August 30, 2024 13:16
@RDaxini RDaxini mentioned this pull request Aug 30, 2024
6 tasks
@RDaxini
Copy link
Contributor Author

RDaxini commented Sep 3, 2024

Marked as draft until #2191 has been addressed to avoid duplicate reviews due to similarities between the changes made in these PRs

@RDaxini RDaxini marked this pull request as draft September 3, 2024 09:02
@RDaxini RDaxini marked this pull request as ready for review September 24, 2024 19:50
@RDaxini RDaxini mentioned this pull request Sep 24, 2024
11 tasks
docs/sphinx/source/whatsnew/v0.11.1.rst Outdated Show resolved Hide resolved
pvlib/irradiance.py Outdated Show resolved Hide resolved
@kandersolar kandersolar added this to the v0.11.1 milestone Sep 24, 2024
pvlib/irradiance.py Outdated Show resolved Hide resolved
moved references from first line to notes section
@cwhanse
Copy link
Member

cwhanse commented Sep 25, 2024

@RDaxini please mark this PR as also closing #1405

NVM I could do it so I did.

@RDaxini
Copy link
Contributor Author

RDaxini commented Sep 26, 2024

@RDaxini please mark this PR as also closing #1405

NVM I could do it so I did.

Thanks! Had no idea that issue existed. I have marked #2191 and #2193 as "related to" as well since it was noted in the discussion that:

Seems like several of those items apply to the other sky diffuse model docstrings as well.

How should I resolve this comment:

To discuss: remove "must be >=0", or change to "should be >=0". The code doesn't require this restriction, although the model certainly isn't valid outside that range. My view: model constraints on input (should be's) would be better placed in the description or in Notes, and code restrictions (must be's) in the parameter descriptions.

My view: if we keep the (recommended) constraint written in the docs, +1 to changing to "should be" and +0.75 on moving to notes/descriptions.

@cwhanse
Copy link
Member

cwhanse commented Sep 26, 2024

How should I resolve this comment:

Let's address it in a follow-on PR. I agree with your proposed rule (model constraints only in the parameter description, other restrictions in the Notes). But I think text like this is found in many places.

The original intent was to inform users of restrictions that would guard against invalid results. That intent wasn't applied consistently, and in places, parameter ranges for model validation were substituted. If we keep ranges in the Notes we need to agree on their purpose.

@RDaxini
Copy link
Contributor Author

RDaxini commented Sep 26, 2024

@cwhanse okay, I understand that better now. Thanks for explaining.
So in that case, I think this PR is read to go:)

@kandersolar kandersolar merged commit 94c3fc5 into pvlib:main Sep 26, 2024
25 of 26 checks passed
@kandersolar
Copy link
Member

Thanks @RDaxini!

@RDaxini RDaxini deleted the update_klucher_docs branch September 26, 2024 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing Descriptions for Formula Components in haydavies Klucher diffuse sky docstring inconsistency
4 participants