Skip to content

Conversation

@Chirag3841
Copy link

@Chirag3841 Chirag3841 commented Nov 26, 2025

(x means done)

  • Closes Add examples for run_model methods, add docstrings for ModelChain constants #1043 (documentation portion)
  • I am familiar with the contributing guidelines
  • Tests added (not required for documentation-only PR)
  • Updates entries in docs/sphinx/source/reference (not required for constants docstrings)
  • Adds description and name entries in "what's new" file (not needed for small internal doc wording)
  • New code is fully documented (docstrings added)
  • Pull request is complete and ready for review
  • Maintainer: assign labels and milestone
    @cwhanse
    I have done changes for PR1.

@Chirag3841
Copy link
Author

Chirag3841 commented Nov 26, 2025

@cwhanse
PR-1
PR Title
Add detailed docstrings for WEATHER_KEYS, POA_KEYS, and TEMPERATURE_KEYS

PR Description
This PR improves the documentation of three ModelChain constants added in #943:

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.
I will add required methods after this (remaining PR).

@Chirag3841 Chirag3841 force-pushed the add-constant-docstrings branch from 2739907 to f6709d0 Compare November 26, 2025 14:34
@cwhanse cwhanse added this to the v0.13.2 milestone Dec 1, 2025
Copy link
Member

@cwhanse cwhanse left a 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.

@Chirag3841 Chirag3841 force-pushed the add-constant-docstrings branch from 93d2319 to cc39285 Compare December 2, 2025 07:30
@Chirag3841
Copy link
Author

@cwhanse
Changes applied. Please review.

@cwhanse
Copy link
Member

cwhanse commented Dec 2, 2025

#2602 and #2603 are still stacked on top of this PR. We can review them sequentially.

@Chirag3841
Copy link
Author

@cwhanse
Thanks for the update. All the PRs (#2601, #2602, #2603) have been rebased and updated.
They can be reviewed sequentially as suggested.

@cwhanse
Copy link
Member

cwhanse commented Dec 3, 2025

@cwhanse Thanks for the update. All the PRs (#2601, #2602, #2603) have been rebased and updated. They can be reviewed sequentially as suggested.

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.

@Chirag3841
Copy link
Author

Chirag3841 commented Dec 3, 2025

@cwhanse
Sure sir, Thanks for the clarification.

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.

Add examples for run_model methods, add docstrings for ModelChain constants

2 participants