-
Notifications
You must be signed in to change notification settings - Fork 1k
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
coerce and rotate pvgis TMY data to desired tz and year #2138
Conversation
- add private function `_coerce_and_rotate_pvgis()` - add `utc_offset` and `coerce_year` params to docstring for `get_pvgis_tmy` - call private function if `utc_offset` is not zero
check if utc_offset and coerce_year work as expected
- remove whitespace - shorter lines
- incorrect syntax for indices
- if february is a leap year, when shifting tz, causes issues - so replace february year with non-leap year
- also fix use ts for timestamp when fixing feb leap year
- lower case "s" not TimeStamp
Tested in this Colab notebook: |
Showing my ignorance of PVGIS: does it always return 8760 timestamps? If PVGIS does return 8760, what should I expect if a user coerces to a leap year? I assume they would get a DatetimeIndex with Feb 29 missing. Second question: if I rotate to UTC-10 ( |
Where is utc-10 is that Hawaii? |
Dunno I just picked a number for illustration. |
Ah. Basically we assume a TMY year is periodic, thus every hour's data is defined in the rotated index. |
- add description and links to issue/pr
- also use np.roll - also make new index and dataframe instead of altering original - removes need to sanitize original index February for leap year - remove calendar import but add numpy and pytz - code much simpler shorter, easier to read
- fix Turin is actually CET which is UTC+1 - be DRY so use variables for test year and tz constants, versus WET and hardcoded - check tz unchanged if default zero utc_offset - use _ output args instead of indexing data[0] - add comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, the PSM3 TMY API does this "roll over" also: https://assessingsolar.github.io/unofficial-psm3-userguide/pages/rollover.html
TMY methods have ways of merging between months from different years. This could be done here also. |
- explain setting utc_offset will roll data to start at Jan 1 midnight
- update arg docstrings - allow user to coerce year even if utc_offset is None or zero - use 1990 as default if utc_offset is not None or zero, but coerce_year was unspecified - add warning comment to be explicit and test identity to avoid unexpected implicit booleaness
- refactor utc_offset everywhere including comments and docstring - add additional test to coerce year even if utc offset is zero or none - change tzname to 'UTC' (versus Etc/GMT or Etc/GMT+0) if replacing with zero utc offset
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. We may want to add similar functionality for other TMY datasets in the future, so I think it's worth @AdamRJensen taking a look too if he has the time.
LGTM2 My perception is that this is fixing a flaw in the PVGIS service. Hopefully other TMY sources align on local time. |
use "optional" vs. "default None" per pvlib#1574 Co-authored-by: Kevin Anderson <kevin.anderso@gmail.com>
rename argument "roll_utc_offset" in whatsnew Co-authored-by: Kevin Anderson <kevin.anderso@gmail.com>
The PVGIS team is currently reworking the entire process, so this is a good time to make requests about features and provide feedback. Tagging @NikosAlexandris |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested it and it seems to work great 🥳
One thing I noticed was that the index name is maintained as "time(UTC)" when utc_roll_offset is None and is removed when utc_roll_offset is specified. Should this be tested? Maybe it doesn't matter
- treat tz=None as UTC - allows get_pvgis_tmy to be simpler - remove unnecessary comments
I renamed the index and added tests in 1e9ee64 |
- ... in docstring of private function pvgis._coerce_and_roll_tmy() - rename tmy_data - name new_index explicitly using pd.DatetimeIndex()
Thanks @mikofski Great to see this new feature in the iotools! |
Thank you for pinging me here. Will look into this as time permits. |
@mikofski Would you have some time to help me better understand the "issue" described in this Notebook ? Thank you. |
Hi @NikosAlexandris , in past versions of |
_coerce_and_roll_pvgis_tmy()
roll_utc_offset
andcoerce_year
params to docstring forget_pvgis_tmy
roll_utc_offset
is notNone
orcoerce_year
is notNone
discussed in #1528 (comment)
Updates entries indocs/sphinx/source/reference
for API changes.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`
).remote-data
) and Milestone are assigned to the Pull Request and linked Issue.