Skip to content

add timezone for package overview #162

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 12 commits into from
Jun 10, 2016

Conversation

mikofski
Copy link
Member

@mikofski mikofski commented May 10, 2016

fixes #156 update "package overview" to use UTC or localized time only (v0.3.0)

  • I'm done for now.

  • adds timezone to coordinates dictionary

    coordinates = [(30, -110, 'Tucson', 700, 'US/Mountain'),
    (35, -105, 'Albuquerque', 1500, 'US/Mountain'),
    (40, -120, 'San Francisco', 10, 'US/Pacific'),
    (50, 10, 'Berlin', 34, 'Europe/Berlin')]

  • also moves times creation inside the loop, because tz_localize(pytz.timezone('US/Mountain')) always raises NonExistentTimeError which is pandas issue 8917

  • NOTE: I tried to use tz_localize with 'Etc/GMT+7' which is equivalent to UTC-0700 (ie: US/Mountain), etc, but that raises TypeError: Already tz-aware, use tz_convert to convert. after first loop so times actually has to go inside the loop anyway, and it's just cleaner to make a new datetime index series each loop and localize it to the actual timezones which are clearer than the GMT timezones.

  • add new test module called test_docs.py - this can probably be removed if doctest sphinx extension is used? but I've never used it so not sure?

  • add test_package_overview to test_docs for this issue, checks if energies are all close to previously calculated, except, that now Tuscon and SF don't agree 😦

  • also for expedience I just copied the doc source over to the test, but I put a todo to use docutils to grab this code, unfortunately, tho, docutils doesn't like the ipython directive so I couldn't figure this out right away, also if using doctest extension, maybe this goes away?

@wholmgren
Copy link
Member

Thanks!

This seems like a good place to demonstrate the use of the Etc/GMT time zones. People will see US/Mountain or similar in many other places, so I think we might as well expose them to another option here. Thoughts? I'm ok with either approach.

I'd definitely like to get some kind documentation testing working, but it's pretty low on my priority list. I think that doctests is probably the way to go, but I also haven't used it. In the spirit of small changes and getting something done sooner than later, I suggest removing the test_docs.py from this PR. In any case, it's not obvious to me why it's failing on the py27-min build.

@mikofski
Copy link
Member Author

I'm okay with the Etc/GMT timezones, that was definitely new to me, so I think you're right about it being a good chance to expose users to them. I'll make the switch and push it up to the PR.

Not sure why test_docs fails py-min, it passed on my machine. The traceback says AssertionError: None.

======================================================================
FAIL: test package overview update to timezone
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/miniconda/envs/test_env/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/home/travis/build/pvlib/pvlib-python/pvlib/test/test_docs.py", line 78, in test_package_overview
    ok_(np.allclose(energies.values, pd.Series(ENERGIES).values))
AssertionError: None

I'll remove the test anyway tho since it doesn't really add much, and it's a bit fragile (so not DRY) since it copies the example from the docs source. IE: if the doc source changes, then this test is worthless. I'll make the change and push it up to the PR.

I might not have time to make the changes today, but definitely by Friday.

I've never used doctest and not sure how to. The ipython directive you're using was totally new to me too. I agree postpone doctest, but I'll add an issue that addresses it so we don't forget.

@wholmgren
Copy link
Member

@mikofski do you think you can switch to Etc/GMT remove the test_docs.py file in the next week? I want to finish 0.3.3 before PVSC.

@wholmgren wholmgren added this to the 0.3.3 milestone May 26, 2016
@mikofski
Copy link
Member Author

mikofski commented Jun 1, 2016

When building docs, I get this warning, I think it's unrelated:

>>>-------------------------------------------------------------------------
Warning in pvlib-python\docs\sphinx\source\package_overview.rst at block ending on line 120
Specify :okwarning: as an option in the ipython:: block to suppress this message
----------------------------------------------------------------------------
pvlib-python\pvlib\clearsky.py:247: VisibleDeprecationWarning: using a non-integer number instead of an integer will result in an error in the future
  g = linke_turbidity_table[latitude_index][longitude_index]
<<<-------------------------------------------------------------------------

Otherwise I the documents look good to me and I think this is ready to merge. 😄

# 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+7'),
Copy link
Member

Choose a reason for hiding this comment

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

+8 in San Fran

@wholmgren
Copy link
Member

I agree that it's good to go, except for changing San Francisco from +7 to +8. Oh, and please add a note and your name or username to the whats new.

@wholmgren
Copy link
Member

Thanks @mikofski!

@wholmgren wholmgren merged commit a8fe59f into pvlib:master Jun 10, 2016
@mikofski mikofski deleted the update_pkg_overview_docs branch June 11, 2016 04:42
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 "package overview" to use UTC or localized time only (v0.3.0)
2 participants