-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add detailed docstrings for WEATHER_KEYS, POA_KEYS, and TEMPERATURE_KEYS #2601
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
base: main
Are you sure you want to change the base?
Conversation
|
@cwhanse PR Description 1)WEATHER_KEYS 2)POA_KEYS 3)TEMPERATURE_KEYS Each constant now includes a detailed docstring explaining its purpose, usage, and the data expected by the ModelChain pipeline. The goal is to improve readability and help new contributors understand how input dictionaries are parsed within ModelChain. |
2739907 to
f6709d0
Compare
cwhanse
left a comment
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.
@pvlib/pvlib-maintainer I think adding comments here is enough for now. These constants are not meant to be modified by users. The documentation page for pvlib.modelchain.ModelChain is already a hot mess with all the methods, so I don't see any value to adding these constants.
@Chirag3841 please address the lint issues.
f6709d0 to
891a4aa
Compare
891a4aa to
93d2319
Compare
93d2319 to
cc39285
Compare
|
@cwhanse |
To be clear, when I look at #2602, I see the edits in #2601 to add docstring comments for constants. It appears that #2602 was created by branching from #2601, or by adding edits to a file that was modified for #2601. If you look at the commit history for #2602 you'll see what I am trying to describe. If we review #2602 and merge without changes, then #2601 becomes redundant. If we make changes to #2601, those changes have to also be applied to #2602, otherwise #2602 will undo them. This is why it is good practice not to mix commits in separate PRs. I want to discuss the docstrings for the module constants with other maintainers, so I'm going to hold off reviewing these PRs for a bit. |
|
@cwhanse |
(x means done)
docs/sphinx/source/reference(not required for constants docstrings)@cwhanse
I have done changes for PR1.