Skip to content

Update intro tutorial (#1116) #1144

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 2 commits into from
Jan 26, 2021
Merged

Update intro tutorial (#1116) #1144

merged 2 commits into from
Jan 26, 2021

Conversation

Peque
Copy link
Contributor

@Peque Peque commented Jan 24, 2021

  • Closes Update ModelChain.run_model documentation #1116
  • I am familiar with the contributing guidelines
  • Tests added (N/A)
  • Updates entries to docs/sphinx/source/api.rst for API changes. (N/A)
  • 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`). (N/A)
  • New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary. (N/A)
  • 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.

This MR updates the example code in the intro tutorial. A couple of things to note:

  • The procedural and OO results match exactly, as expected
  • Instead of clearsky, we are using TMY data from PVGIS, highlighting the use of that iotools function

Also, I could add some transformations to the downloaded TMY data to avoid possible confusion to new users, in particular:

  • Replace TMY index year with 2019, so in case the user plots the energy series, they see a continuous year
  • Convert TMY index to local timezone, so in case the user plots the hourly energy, they see a more expected curve

Make the procedural and OO results match exactly and highlight the use
of `iotools` to download meteorological data.
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.

Thanks @Peque this is great. One comment below and can you also add a note to the 0.9 whats new file?

includes irradiation, temperature and wind speed. Note that PVGIS uses
different naming conventions, so it is required to rename the weather data
variables before using them. PVGIS weather data is already UTC-localized, so
conversion to local timezone is optional.
Copy link
Member

Choose a reason for hiding this comment

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

The text itself is pretty neutral, but I don't want to imply that we favor PVGIS. I'm wondering if we need a sentence here like See :ref:`iotools` for other weather data sources. or maybe even something more explicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed a fixup commit with a little change. Feel free to rephrase it as you'd like if you still prefer some other way to say it. 😊

@Peque Peque requested a review from wholmgren January 25, 2021 17:47
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.

Thanks @Peque .

My only comment is outside the scope of this tutorial: it is unfortunate that the key for getting 'sandia' inverter parameters using retrieve_sam is 'cecinverter'. "sandia", "SAM" and "cec" all appear in this line of code; no surprise that novices get confused.

@Peque
Copy link
Contributor Author

Peque commented Jan 25, 2021

By the way, just in case you missed my comment above, I could add some transformations to the downloaded TMY data to avoid possible confusion to new users, in particular:

  • Replace TMY index year with 2019, so in case the user plots the energy series, they see a continuous year (PVGIS assigns a datetime with the year it got the typical meteorological month data from)
  • Convert TMY index to local timezone, so in case the user plots the hourly energy, they see a more expected curve

Like this:

--- a/docs/sphinx/source/introtutorial.rst
+++ b/docs/sphinx/source/introtutorial.rst
@@ -71,7 +71,11 @@ includes irradiation, temperature and wind speed.
         latitude, longitude, name, altitude, timezone = location
         weather = pvlib.iotools.get_pvgis_tmy(latitude, longitude)[0]
         weather = weather.rename(columns=variables_translation)
+        weather.index = weather.index.map(lambda t: t.replace(year=2019))
         weather.index.name = "utc_time"
+        # Optional conversion to local timezone
+        weather.index = weather.index.tz_convert(timezone)
+        weather.index.name = "local_time"
         tmys.append(weather)

Or would you rather leave it as UTC and as isolated months from different years?

@wholmgren
Copy link
Member

let's keep it as is - the "intro example" can't do everything. thanks @Peque!

@wholmgren wholmgren merged commit 7ed18c4 into pvlib:master Jan 26, 2021
@EricDuminil
Copy link
Contributor

Excellent, thanks for the work!

It might be a good time to correct the inaccurate coordinates in introtutorial.rst.
Right now, San Francisco is in Reno and Berlin is almost in France.

If you're interested:

     import pandas as pd
     import matplotlib.pyplot as plt
 
-    # very approximate
     # latitude, longitude, name, altitude, timezone
     coordinates = [
-        (30, -110, 'Tucson', 700, 'Etc/GMT+7'),
-        (35, -105, 'Albuquerque', 1500, 'Etc/GMT+7'),
-        (40, -120, 'San Francisco', 10, 'Etc/GMT+8'),
-        (50, 10, 'Berlin', 34, 'Etc/GMT-1'),
+        (32.2, -111.0, 'Tucson', 700, 'Etc/GMT+7'),
+        (35.1, -106.6, 'Albuquerque', 1500, 'Etc/GMT+7'),
+        (37.8, -122.4, 'San Francisco', 10, 'Etc/GMT+8'),
+        (52.5, 13.4, 'Berlin', 34, 'Etc/GMT-1'),
     ]

Here's the commit:
https://github.com/EricDuminil/pvlib-python/commit/7abbfc8e7d9c4a59ee1b937948cb4eefeaab1801

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.

Update ModelChain.run_model documentation
4 participants