-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Revise modelchain.rst for multiple arrays, results attribute #1223
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
Conversation
docs/sphinx/source/modelchain.rst
Outdated
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
||
The PVSystem can represent a PV system with a single array of modules, or | ||
with [multiple arrays](#PVSystem and Arrays). The same models are applied to |
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.
Anyone know how to link to a heading in a different file (pvsystem.rst, in this case)?
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.
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.
thanks for the careful edits
Haven't looked at the new text yet, but I did quickly scroll through the log on the last RTD build and it looks like |
I will search the rest of the doc files. |
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.
Mostly nitpicks about sphinx output
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 carefully read the changes and they are great. Thanks @cwhanse. Minor comments below but fine with me to merge anytime.
docs/sphinx/source/modelchain.rst
Outdated
logic to assign one of the model-specific methods, such as | ||
:py:meth:`~pvlib.modelchain.ModelChain.sapm` or :py:meth:`~pvlib.modelchain.ModelChain.snlinverter`, | ||
to the universal method names ``ModelChain.dc_model`` and ``ModelChain.ac_model``, | ||
respectively. A few examples are shown below. Inferrence methods generally work |
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.
this suggestion might have been buried/overlooked.
Builds clean on my machine. I say merge it now. We'll need a doc clean up PR for a few unrelated problems. |
ready to merge @cwhanse? |
Yes, ready. |
Tests addedUpdates entries todocs/sphinx/source/api.rst
for API changes.Adds description and name entries in the appropriate "what's new" file indocs/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`
).New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.