Skip to content

Localized time index not working when using 'nrel_c' method #237

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

Closed
anomam opened this issue Aug 12, 2016 · 2 comments · Fixed by #333
Closed

Localized time index not working when using 'nrel_c' method #237

anomam opened this issue Aug 12, 2016 · 2 comments · Fixed by #333
Labels
Milestone

Comments

@anomam
Copy link
Contributor

anomam commented Aug 12, 2016

Hi,

I'm not sure if that was already covered in an other issue (couldn't find it) or intentional in the code, but when using the function get_solarposition() with method='nrel_c', it doesn't seem to matter whether the time index is localized or not.
The function will assume that the time index is at 'UTC' no matter what. Which goes against what was described in http://pvlib-python.readthedocs.io/en/latest/timetimezones.html.

So I ended up converting my timezone to 'UTC' before calculation, and then back to local after the calculation

Example:
pvlib_loc = location.Location(latitude, longitude, tz='Etc/GMT+8')
localized_index = index.tz_localize('Etc/GMT+8')
utc_index = localized_index.tz_convert('UTC')
solarAngles = pvlib_loc.get_solarposition(utc_index, method='nrel_c')
solarAngles.index = localized_index

@wholmgren
Copy link
Member

I'd welcome a pull request if you want to fix it or at least document the bug. Personally, I'd vote for removing the nrel_c option all together, but that's probably a separate issue. Is there a reason you're using nrel_c instead of the nrel_numpy or nrel_numba options?

@wholmgren wholmgren added the bug label Aug 17, 2016
@wholmgren wholmgren added this to the 0.4.1 milestone Aug 17, 2016
@wholmgren wholmgren modified the milestones: Someday, 0.4.1 Oct 4, 2016
@mikofski
Copy link
Member

mikofski commented Nov 4, 2016

At the very least this should be changed in the docstring I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants