Skip to content

Improve docstring in IEC 61853 module temperature model #840

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

Closed
wants to merge 3 commits into from
Closed

Improve docstring in IEC 61853 module temperature model #840

wants to merge 3 commits into from

Conversation

adriesse
Copy link
Member

@adriesse adriesse commented Dec 18, 2019

  • Closes Implement IEC 61853 module temperature model #750
  • 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.

@adriesse
Copy link
Member Author

@cwhanse Could you check this?

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.

Optional: perhaps add the subscript indicator, and clarify that equation 5 refers to [1].

@cwhanse cwhanse added this to the 0.7.1 milestone Dec 18, 2019
@wholmgren
Copy link
Member

Just to confirm... Are you happy with the way the docstring is rendered? See the continuous-documentation/read-the-docs link in the checks. It's not what I'd choose to do but I'm ok merging.

@adriesse
Copy link
Member Author

Sorry I don't see that link and I don't know how the doc string looks. :(

@wholmgren
Copy link
Member

Do you see it if you click "show all checks"?

Screen Shot 2019-12-19 at 10 52 21 AM

Screen Shot 2019-12-19 at 10 53 55 AM

@adriesse
Copy link
Member Author

Thanks, yes, not nice. I would remove the three underscores tomorrow unless Cliff objects.

Also, the units for u1 and u_v in pvsyst_cell look suspicious...

@kandersolar
Copy link
Member

@adriesse ...in equation 5 of [1]_... is rendering the underscore because the citation it references isn't formatted in a way that numpydoc recognizes. Here's an example of compatible reference formatting (see the leading two dots) example.

You might also want to use sphinx's :math: notation to interpret U’_0 and U’_1 as Latex code instead of plaintext: example

@adriesse
Copy link
Member Author

I understand, and I appreciate the hints all around, but this simple clarification is again turning into way more than I bargained for.

@adriesse adriesse closed this Dec 19, 2019
@adriesse adriesse deleted the faiman2 branch April 11, 2022 15:51
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.

Implement IEC 61853 module temperature model
4 participants