Skip to content
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

issue #2314 updating doc #2382

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bandish1304
Copy link

  • [x ] Closes #xxxx
  • [x ] I am familiar with the contributing guidelines
  • [x ] Updates entries in docs/sphinx/source/reference for API changes.
  • [x ] 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`).
  • [x ] Pull request is nearly complete and ready for detailed review.
  • [x ] Maintainer: Appropriate GitHub Labels (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

Copy link
Contributor

@RDaxini RDaxini left a comment

Choose a reason for hiding this comment

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

Thanks for working on this issue.
Made a couple of suggestions following an initial scan through. It might be helpful to review the contributing guidelines, in particular the specific page on documentation and style (e.g. regarding units and definitions)
We could get into some of the more involved aspects of this PR afterwards.

* `aoi`: The angle-of-incidence of direct irradiance onto the
rotated panel surface. [degrees]
rotated panel surface.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
rotated panel surface.
rotated panel surface. [°]

The units should not be deleted from the docs. It's important to state the units for each variable. In this case, we could change from the text "degrees" to the symbol, but not delete it entirely.

* `surface_tilt`: The angle between the panel surface and the earth
surface, accounting for panel rotation. [degrees]
surface, accounting for panel rotation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please refer to the variable defiitions in the glossary, for example for surface_tilt: https://pvlib-python.readthedocs.io/en/latest/user_guide/nomenclature.html#term-surface_tilt
You can also link definitions on the docs page to glossary definitions using the :term: role, for example:

Suggested change
surface, accounting for panel rotation.
surface, accounting for panel rotation. See :term:`surface_tilt`.

You can find other similar guidelines on the contributing page

positive cross-axis tilt if the tracker axes plane slopes down to the
west. Use :func:`~pvlib.tracking.calc_cross_axis_tilt` to calculate
``cross_axis_tilt``. [degrees]
Ground coverage ratio, the ratio of row width to row spacing.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the more comprehensive definition has value. Perhaps finding a midpoint for the docs then having a more comprehensive definition on the glossary page is an option?

@RDaxini
Copy link
Contributor

RDaxini commented Feb 10, 2025

@bandish1304 another suggestion, you can tag the issue number where the xxx is in the issue description, and change the title to something that summarizes the issue you are fixing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants