Skip to content

Conversation

@kandersolar
Copy link
Member

@kandersolar kandersolar commented Jul 13, 2021

  • Code changes are covered by tests
  • New functions added to __init__.py
  • API.rst is up to date, along with other sphinx docs pages
  • Example notebooks are rerun and differences in results scrutinized
  • Updated changelog

This PR addresses a failure in the clear-sky tamb function with pandas 1.3.0 (noticed in the eager-upgrade CI job on #281) and suppresses a bunch of deprecation warnings for casting datetime indexes to int dtype:

  • AttributeError: 'numpy.float32' object has no attribute 'is_integer'
  • FutureWarning: casting timedelta64[ns] values to int64 with .astype(...) is deprecated and will raise in a future version. Use .view(...) instead.

Note that the remaining astype/view warnings are from pvlib and have already been addressed on that end: pvlib/pvlib-python#1256

@kandersolar kandersolar requested a review from mdeceglie July 13, 2021 22:50
@cdeline
Copy link
Collaborator

cdeline commented Jul 15, 2021

I've tested this out and it seems to work, although I couldn't explain exactly how or why it works. I've seen other projects taking this approach to move from .astype to .view because it is suggested by the deprecation warning. I will say that using the .view function seems very kludgy and un-Pythonic, and I wonder if there isn't a different way to explicitly convert from datetime to epoch nanoseconds. All of the other changes you have here are just fine though.

Copy link
Collaborator

@mdeceglie mdeceglie left a comment

Choose a reason for hiding this comment

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

Though the pandas changes necessitating this are still somewhat mysterious to me, these changes all look good. Thanks @kanderso-nrel

@mdeceglie mdeceglie merged commit b6dcdd2 into master Jul 16, 2021
@mdeceglie mdeceglie deleted the pandas_130 branch July 16, 2021 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants