Skip to content

solarposition.hour_angle is inefficient and fails on some pandas versions #598

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.

Sign up for GitHub

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

Jump to bottom
Closed
wholmgren opened this issue Oct 4, 2018 · 4 comments · Fixed by #599
Closed

solarposition.hour_angle is inefficient and fails on some pandas versions #598

wholmgren opened this issue Oct 4, 2018 · 4 comments · Fixed by #599
Labels
bug
Milestone
0.6.1

Comments

@wholmgren
Copy link
Member

Describe the bug
For some reason related to timezone handling, the solarposition.hour_angle function fails on pandas versions 0.15-0.18. We test 0.14 and latest. Apparently we were on pandas 0.19 or greater when this function was created.

Also, the loop over datetime values is probably slow.

    hours = np.array([(t - t.tz.localize(
        dt.datetime(t.year, t.month, t.day)
    )).total_seconds() / 3600. for t in times])

I think this gives the same result:

hours = (times.astype(int) - times.normalize().astype(int)) / 1e9 / 3600

Versions:

  • pvlib.__version__: 0.6.0
  • pandas.__version__:
  • python:

Additional context
probably should address #597 first
@wholmgren wholmgren added the bug label Oct 4, 2018
@wholmgren wholmgren added this to the 0.6.1 milestone Oct 4, 2018
@wholmgren wholmgren mentioned this issue Oct 4, 2018
Merged
8 tasks
@mikofski
Copy link
Member

mikofski commented Oct 4, 2018

@wholmgren I believe this may be related to the pandas issues I observed in #583 - I noticed this hour_angle code, and I thought I should have addressed it then, but I didn't. It was my bad code from the Bird clear sky model, sorry!

@wholmgren
Copy link
Member Author

No worries, Mark. Do you want to address this and #597 in #583? Or do you think we should handle them separately?

@mikofski
Copy link
Member

mikofski commented Oct 4, 2018

I will address them, but I'd rather open a new PR to fix the hour_angle issues, unless you would prefer not

@wholmgren
Copy link
Member Author

wholmgren commented Oct 4, 2018 •
edited
Loading

New PR sounds good to me. Thanks!

mikofski added a commit to mikofski/pvlib-python that referenced this issue Oct 5, 2018
@mikofski
add test for hour_angle, vectorize
ace0a17 
* closes pvlib#598 vectorize to make it more efficient
* closes pvlib#597 add test

Signed-off-by: Mark Mikofski <[email protected]>
@mikofski mikofski mentioned this issue Oct 5, 2018
Merged
8 tasks
wholmgren pushed a commit that referenced this issue Oct 5, 2018
@mikofski @wholmgren
add test for hour_angle, vectorize (#599)
5ee82b1 
* add test for hour_angle, vectorize

* closes #598 vectorize to make it more efficient
* closes #597 add test

Signed-off-by: Mark Mikofski <[email protected]>

* BUG: use np.int64 works better for older numpy version than python int

* also converting times to int before subtracting works better for older
pandas versions which were not calculating the timedeltas correctly
* remove comment with missing space after hash, add FIXME that explains
why the expected values are slightly different than the SPA calculator
output

* fix hanging indent

* BUG: replace utcoffset() with reliable, efficient approach ...

* ... suggested by @wholmgren (thx!)
* utcoffset() is unpredictable when used with pandas datetime indices
* only predictable with Python datetime objects or pandas Timestamps
* instead replace tzinfo with None to get naive local times, and
calculate difference from tz-aware times to get timezones

* BUG: combine arithmetic to make calculation more efficient

* also use asarray wrapper at return to ensure consistency
* add comments to explain to future maintainers

* stickler

* remove try-except, doesn't work anyway, wait for pandas >=0.15.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
0.6.1
Development

Successfully merging a pull request may close this issue.

add test for hour_angle, vectorize mikofski/pvlib-python
2 participants
@mikofski @wholmgren