Skip to content

fix for https://github.com/pvlib/pvlib-python/issues/102 #103

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 31 commits into from
Jan 20, 2016

Conversation

dacoex
Copy link
Contributor

@dacoex dacoex commented Dec 1, 2015

inserted link to variables

inserted link to variables
@dacoex dacoex mentioned this pull request Dec 1, 2015
@wholmgren
Copy link
Member

Thanks. Perhaps I wasn't clear enough in #102... Since you seem particularly interested in improving the documentation, I recommend that you set up readthedocs.org to render your fork of pvlib. This will make it easier to review this PR and future PRs. If you're just making small edits to the docs then I don't think it's needed, but here it will be helpful to see the rendered version since you're making new files and tables.

It is pretty simple to make an account on readthedocs, hook it up to your github account, and then tell rtd to render your fork and possibly your doc_var branch. See here:

http://docs.readthedocs.org/en/latest/getting_started.html#import-your-docs

Then post a comment here with a link to your version of the new docs.

@dacoex
Copy link
Contributor Author

dacoex commented Dec 2, 2015

@wholmgren
Copy link
Member

Thanks, that is helpful. A few comments:

  1. The page name is "Variables and style rules" but it currently only has a table of variables. We need to either just have the table of variables and change the name or we need to include some text about style rules and keep the name as is. I don't have an opinion on what's best.
  2. I'd replace "rules" with "conventions".
  3. Do we need to keep the "comments" column? I'd say no.
  4. Do we want to keep the wiki page after merging this? I'd say delete it so that we don't have to make these changes in two places.

@dacoex
Copy link
Contributor Author

dacoex commented Dec 3, 2015

OK. 1 question to my earlier comment 👍

Additionally, the csv values can be reused, e.g. to assign variables or translate variables into >descriptions.

We could add the table to the folder
https://github.com/pvlib/pvlib-python/tree/master/pvlib/data

Then, we could read the variables in and make descriptive lables for graphs etc from it:

long_name = variable_csv_file['dni']

@wholmgren wholmgren added this to the 0.3 milestone Dec 3, 2015
@wholmgren
Copy link
Member

We could add the table to the folder
https://github.com/pvlib/pvlib-python/tree/master/pvlib/data Then, we could read the variables in and make descriptive lables for graphs etc from it: long_name = variable_csv_file['dni']

Maybe. I doubt that many people would take advantage of something like that, but it probably doesn't hurt anything. We'd probably want to change the multiple-valued cells if we did this.

@dacoex
Copy link
Contributor Author

dacoex commented Jan 5, 2016

@wholmgren I will move the table
https://github.com/pvlib/pvlib-python/tree/master/pvlib/data

this makes renaming to the agreed variables easier.

@wholmgren
Copy link
Member

@dacoex you should rebase this and rebuild your docs now that #108 has been merged.

@@ -69,7 +69,7 @@ coverage.xml


#Datafiles
*.csv
#*.csv
Copy link
Member

Choose a reason for hiding this comment

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

No need to change this.

@wholmgren
Copy link
Member

@dacoex thanks for the updates. Please see the line notes above.

@dacoex
Copy link
Contributor Author

dacoex commented Jan 19, 2016

all proposals where I did not add a counter comment above are noew implemented.

Probably left with the table wrap issue.

saturation_current;diode saturation current
resistance_series;series resistance
resistance_shunt;shunt resistance
tf;transposition factor, the gain ration of the radiation in inclinded plane to horizontal plane: :math:`\frac{poa\_global}{ghi}`
Copy link
Member

Choose a reason for hiding this comment

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

multiple typos in this line

Copy link
Member

Choose a reason for hiding this comment

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

I prefer transposition_factor to tf.

@wholmgren
Copy link
Member

Doesn't look like it's rendering right now.

http://pvlib-python-dacoex.readthedocs.org/en/latest/variables_style_rules.html

@dacoex
Copy link
Contributor Author

dacoex commented Jan 19, 2016

looks good now, I hope. CU tomorrow. I am in another tz (UTC+1)

@wholmgren
Copy link
Member

Looks good, thanks.

wholmgren added a commit that referenced this pull request Jan 20, 2016
@wholmgren wholmgren merged commit f2b5177 into pvlib:master Jan 20, 2016
@dacoex
Copy link
Contributor Author

dacoex commented Jan 20, 2016

Thanks for the patience. I can only learn from this peer review. And it increases my confidence in the code.

Next time tell me and I will stash the trial and error together.

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.

2 participants