Skip to content

Docstring for ModelChainResult #1233

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

Merged
merged 15 commits into from
May 17, 2021
Merged

Conversation

cwhanse
Copy link
Member

@cwhanse cwhanse commented May 16, 2021

  • Closes Improve documentation of ModelChainResult class #1126
  • I am familiar with the contributing guidelines
  • Tests added
  • Updates entries to docs/sphinx/source/api.rst for API changes.
  • 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`).
  • New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
  • Pull request is nearly complete and ready for detailed review.
  • Maintainer: Appropriate GitHub Labels and Milestone are assigned to the Pull Request and linked Issue.

@cwhanse cwhanse mentioned this pull request May 16, 2021
24 tasks
@cwhanse cwhanse added this to the 0.9.0 milestone May 16, 2021
@cwhanse cwhanse marked this pull request as ready for review May 16, 2021 16:33
Copy link
Member

@kandersolar kandersolar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PerArray makes an undesirable appearance in the list of attributes (link); should we rename it to _PerArray?

airmass: Optional[pd.DataFrame] = field(default=None)
"""Air mass in a DataFrame containing columns ``'airmass_relative'``,
``'airmass_absolute'`` (unitless); see
:py:meth:`~pvlib.location.get_airmass` for details."""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
:py:meth:`~pvlib.location.get_airmass` for details."""
:py:meth:`~pvlib.location.Location.get_airmass` for details."""

Copy link
Member

@wholmgren wholmgren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is great!

losses: Optional[Union[pd.Series, float]] = field(default=None)
"""Series (or tuple of Series, one for each array) containing DC loss
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

never a tuple

times: Optional[pd.DatetimeIndex] = None
"""DatetimeIndex (or tuple of DatetimeIndex, one for each array) contains
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

never a tuple

@wholmgren
Copy link
Member

wholmgren commented May 16, 2021

PerArray makes an undesirable appearance in the list of attributes (link); should we rename it to _PerArray?

type linters complain about the line _T = TypeVar('T') - they want it to be T = TypeVar('T'). I assume that would mean the generic type var would also show up in the docs. So maybe there is a way to exclude the attributes? Or move them to the module scope?

@kandersolar
Copy link
Member

PerArray makes an undesirable appearance in the list of attributes (link); should we rename it to _PerArray?

type linters complain about the line _T = TypeVar('T') - they want it to be T = TypeVar('T'). I assume that would mean the generic type var would also show up in the docs. So maybe there is a way to exclude the attributes? Or move them to the module scope?

Moving to module scope seems to solve the problem, although I did not get mypy's opinion about it. I looked around a bit for how other packages document dataclasses and came up mostly short -- this is the only example I found, which uses the standard approach of docstring sections: https://github.com/scipy/scipy/blob/701ffcc8a6f04509d115aac5e5681c538b5265a2/scipy/stats/_relative_risk.py#L21

@wholmgren
Copy link
Member

wholmgren commented May 16, 2021

I'd also like to see the type hints removed from the class signature. Nobody is going to directly instantiate this class, but all that text could be distracting/intimidating. My reading is that we just need to add autodoc_typehints = 'none' to conf.py (see sphinx docs) but that isn't working for me. Might be specific to the dataclass init - maybe find a way to skip the signature entirely?

@cwhanse
Copy link
Member Author

cwhanse commented May 16, 2021

I'd also like to see the type hints removed from the class signature. Nobody is going to directly instantiate this class, but all that text could be distracting/intimidating. My reading is that we just need to add autodoc_typehints = 'none' to conf.py (see sphinx docs) but that isn't working for me. Might be specific to the dataclass init - maybe find a way to skip the signature entirely?

I agree that text is distracting, but I have no idea how to suppress it either.

@cwhanse
Copy link
Member Author

cwhanse commented May 16, 2021

@wholmgren hints to controlling a dataclass docstring here?

Copy link
Member

@wholmgren wholmgren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not worry about the signature in this pr.

Comment on lines 249 to 252
_T = TypeVar('T')


_PerArray = Union[_T, Tuple[_T, ...]]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No underscore on either of these. Type variables should be public so other libraries that build on pvlib can inspect them and benefit from them.

Copy link
Member

@wholmgren wholmgren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merge when green.

@cwhanse cwhanse merged commit 7eae1fc into pvlib:master May 17, 2021
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.

Improve documentation of ModelChainResult class
3 participants