Update irradiance.klucher docs

pvlib / pvlib-python

A set of documented functions for simulating the performance of photovoltaic energy systems.

https://pvlib-python.readthedocs.io

BSD 3-Clause "New" or "Revised" License

1.17k stars 991 forks source link

Update irradiance.klucher docs #2192

Closed RDaxini closed 3 days ago

RDaxini commented 1 month ago

[X] Partially closes #2183, closes #1405
[X] I am familiar with the contributing guidelines
[x] Adds description and name entries in the appropriate "what's new" file in 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`).
[X] New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
[x] Pull request is nearly complete and ready for detailed review.
[x] Maintainer: Appropriate GitHub Labels (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

Docs here

RDaxini commented 3 weeks ago

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

cwhanse commented 4 days ago

@RDaxini please mark this PR as also closing #1405

NVM I could do it so I did.

RDaxini commented 4 days ago

@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 commented 3 days ago

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 commented 3 days ago

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

kandersolar commented 3 days ago

Thanks @RDaxini!