Skip to content

Conversation

@AdamRJensen
Copy link
Member

@AdamRJensen AdamRJensen commented May 16, 2023

  • Closes SolarAnywhere File -- pvlib.iotools.read_tmy3 Bug #1732
  • I am familiar with the contributing guidelines
  • Tests added
  • Updates entries in docs/sphinx/source/reference for API changes.
  • 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`).
  • New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
  • Pull request is nearly complete and ready for detailed review.
  • Maintainer: Appropriate GitHub Labels (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

Add optional encoding parameter to the read_tmy3 function.

@AdamRJensen AdamRJensen requested a review from kandersolar May 16, 2023 18:07
Copy link
Member

@kandersolar kandersolar left a comment

Choose a reason for hiding this comment

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

Thanks @AdamRJensen for the PR and @andylam598 for the bug report!

@AdamRJensen AdamRJensen added the remote-data triggers --remote-data pytests label May 16, 2023
@AdamRJensen
Copy link
Member Author

AdamRJensen commented May 17, 2023

@kandersolar Setting encoding='utf-8-sig' does solve the problem originally raised in #1732. However, the file uploaded by @andylam598 also had the leading zeros stripped from the time column. The column should have the format HH:MM, e.g., 00:00, but rather with the leading zeros stripped the same value would be 0:00. This resulted in another issue given that the hour is extracted based on position:

shifted_hour = data['Time (HH:MM)'].str[:2].astype(int) % 24

While we shouldn't necessarily be able to handle files that are incorrectly formatted (e.g., missing zeros), it seems more robust to use split:

shifted_hour = data['Time (HH:MM)'].str.split(':').str[0].astype(int) % 24

It seems that it would make sense to also parse the minute part of the time column, just in case some files have an offset - what do @pvlib/pvlib-core think?

Copy link
Member

@kandersolar kandersolar left a comment

Choose a reason for hiding this comment

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

LGTM

@AdamRJensen AdamRJensen added remote-data triggers --remote-data pytests and removed remote-data triggers --remote-data pytests labels May 26, 2023
@kandersolar kandersolar merged commit 75d800a into pvlib:main May 26, 2023
@AdamRJensen AdamRJensen deleted the expose_encoding_tmy3 branch May 26, 2023 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement io remote-data triggers --remote-data pytests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SolarAnywhere File -- pvlib.iotools.read_tmy3 Bug

2 participants