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

Documentation improvement #335

Closed
birgits opened this issue Jun 8, 2017 · 3 comments
Closed

Documentation improvement #335

birgits opened this issue Jun 8, 2017 · 3 comments
Milestone

Comments

@birgits
Copy link
Contributor

birgits commented Jun 8, 2017

I just stumbled across two things in the documentation where it would be helpful to correct them:

  • For the surface_tilt in the PVSystem class it says:

Up=0, horizon=90.

This is a bit confusing since I guess it is meant as defined in the basic_chain:

The tilt angle is defined as degrees from horizontal (e.g. surface facing up = 0, surface facing horizon = 90)

Is that right? Maybe we could use the same definition in the PVSystem class.

  • In the docstring of the Modelchain class the default value for the orientation_strategy south_at_latitude_tilt is missing.
@wholmgren
Copy link
Member

Is that right?

That is right. It could be updated in a lot of places. Many other parameters could be made more consistent.

In the docstring of the Modelchain class the default value for the orientation_strategy south_at_latitude_tilt is missing.

We generally have not put default values in the doc strings. I think they're useful, though, so feel free to add this. I'd only ask that we add defaults to all parameters of a function, instead of only a single parameter. See also #290.

@uvchik
Copy link
Contributor

uvchik commented Jun 12, 2017

According to #290 we may want to add a comment to the docstring entry of orientation_strategy:

(default: 'south_at_latitude_tilt') WARNING: The default will be set to None from version 0.5.x on.

def __init__(self, system, location,
-            orientation_strategy='south_at_latitude_tilt',
+            orientation_strategy='old_default`,
             clearsky_model='ineichen',
             ....

+     if orientation_strategy == 'old_default'
+        orientation_strategy='south_at_latitude_tilt'
+        warnings.warn("Default value will be changed.... ", FutureWarning)

People can switch of this warning by defining the parameter explicitly and so they ready for the changes.

@wholmgren
Copy link
Member

closed by #336

@wholmgren wholmgren modified the milestones: 0.4.6, 0.5.0 Aug 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants