Skip to content

BUG: constrain cftime to >=1.1.1 #947

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

Merged
merged 22 commits into from
Apr 13, 2020
Merged

BUG: constrain cftime to >=1.1.1 #947

merged 22 commits into from
Apr 13, 2020

Conversation

CameronTStark
Copy link
Contributor

@CameronTStark CameronTStark commented Mar 30, 2020

  • Closes BUG: netcdf4 conversion error in forecast.py  #944
  • I am familiar with the contributing guidelines
  • 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`).
  • Pull request is nearly complete and ready for detailed review.
  • Maintainer: Appropriate GitHub Labels and Milestone are assigned to the Pull Request and linked Issue.

Limiting cftime<=1.1.0 . This is a few releases ahead of the suggested 1.0.4 but still passes all tests.

@CameronTStark CameronTStark added this to the 0.7.2 milestone Mar 30, 2020
@CameronTStark
Copy link
Contributor Author

It's good that everything checked out on this run. I reran the failing tests from #937 and they still fail confirming that this is indeed a fix.

I'll add to what's new and this should be ready for approval thereafter.

@CameronTStark
Copy link
Contributor Author

CameronTStark commented Mar 30, 2020

This is ready for review. Edit: needs more work

This is by no means the solution and an issue to address needs to be drafted unless #920 encompasses it. I'll make the issue unless If someone who knows the inner workings of forecast.py and/or how this interacts with it can describe what needs to be done.

@CameronTStark CameronTStark changed the title WIP: BUG:limit cftime to <=1.1.0 for all but Python3.5 BUG:limit cftime to <=1.1.0 for all but Python3.5 Mar 30, 2020
@CameronTStark
Copy link
Contributor Author

As proof of the error I pushed after reverting this branch to master to get the test results. Link to test results

The error is fixed in Python3.6 and above (thanks @kanderso-nrel!) but persists in Python3.5 even with the changes. Link to 3.5 error

cftime only supports >=3.6 and therefore doesn't have the kwarg only_use_python_datetimes for the downloaded version in the 3.5 test.

The Test_conda_linux environment for Python3.5 installs 1.0.0b1. Link to installed packages for 3.5 test

All other Python versions install the most current 1.1.1 Link to installed packages for 3.6 test

The most obvious options are:

  • Go back to pinning 1.1.0
  • Drop support for Python3.5

Are there any more options?

@wholmgren
Copy link
Member

Pinning is ok with me, but Another option is to xfail or skip the test if run on 3.5. I’d rather wait until 0.8 to drop 3.5.

@CameronTStark CameronTStark changed the title BUG:limit cftime to <=1.1.0 for all but Python3.5 BUG:limit cftime to >=1.1.1 Apr 1, 2020
@CameronTStark CameronTStark changed the title BUG:limit cftime to >=1.1.1 WIP: BUG:limit cftime to >=1.1.1 Apr 1, 2020
@CameronTStark CameronTStark changed the title WIP: BUG:limit cftime to >=1.1.1 BUG: constrain cftime to >=1.1.1 Apr 3, 2020
@CameronTStark
Copy link
Contributor Author

CameronTStark commented Apr 3, 2020

I think this, after much learning, is ready for review.

Thanks for your patience for the time it took and notification spam.

Please let me know if the codecov/patch failure needs to be addressed for test. I don't think so but there may be some historical context in which I'm unaware.

Edit:
I was also wondering if there should be a warning in get_data() for 3.5 users when this fails. If so, what's standard practice?

@wholmgren
Copy link
Member

ok to ignore this codecov/patch failure. Ok with me to just let it break on python 3.5 if we're no longer supporting it in pvlib 0.8.

@CameronTStark
Copy link
Contributor Author

When you get a moment @wholmgren can you please merge this? Thanks!

@wholmgren wholmgren merged commit 73e2e0c into pvlib:master Apr 13, 2020
@CameronTStark CameronTStark deleted the limit_cftime branch April 13, 2020 15:25
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 this pull request may close these issues.

BUG: netcdf4 conversion error in forecast.py
2 participants