Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
make test_psm3.py robust to API overuse errors #873
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
make test_psm3.py robust to API overuse errors #873
Changes from 70 commits
e81e941
63c489d
3ef167e
ecb8f79
0f13335
31896ca
5c69773
c348fad
6a48631
c56f8ea
60323df
83e35d3
ae825be
5ce5f8f
6d33c6c
2596c4f
b16d036
edb303c
8a8013c
aacb1d6
1a7ac11
bc37f2f
8736f62
4f11bb5
f7620b6
937f050
27b370c
6068d83
672ec66
572a861
cea3eda
2c8370d
1ef3af6
e764bfa
78ea6a6
b57732a
7a1b5a1
5380567
ea8acd5
4f59d27
51965e2
13045fb
6358642
efe0f1c
65c2d75
b9981c4
5d7c587
0963441
ef56cce
09b0af6
8db513d
89c5af3
9aeabf8
6646a2e
075de57
15a980d
e49cba1
76c9a5b
4468bcb
9927ef5
e1b0959
3237f8e
5c68358
b8d536c
72e3880
6cdc186
be8eca0
33fe42e
6d9230e
07cbcc7
cdff1af
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
can you remind me why all of these tests need pandas 0.22?
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 was hoping you could tell me =)
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'll try taking them off locally and see what errors pop out.
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.
@CameronTStark see this comment in #694
So since we've dropped Python-2.7 support, we just need to check if it's still valid for Python-3.5, otherwise we can remove these.
Also since we bumped pandas to v0.18.1 in pvlib-0.7.0 we can probably definitely drop these now.
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.
Yep, the ability to assemble a datetimes from a sequence
(year, month, ...)
was new in pandas-0.18There 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.
Thanks for researching this. I took off the decorators locally to run the tests and didn't have an issue with my pandas==1.0.1 environment. I tried setting up an environment with 0.18.1 and couldn't get it to install so I can't confirm for that scenario.
Like the
pytest-remotedata
I think this should be addressed in another issue so we can get this across the finishline.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.
@CameronTStark please make follow up issues for these infrastructure topics.
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.
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.
do we want to keep this leap day behavior check? I'm ok getting rid of it but thought it was worth double checking. It's less redundant than the other tests that I questioned earlier.
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.
@kanderso-nrel ?
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.
A colleague of mine reached out the other day asking if I knew why his homemade PSM3 API wrapper didn't work anymore and it turned out that the API rejects the default urllib user-agent string now. Between that and the 30-minute timestamp shift from several months back, I'd have a preference for "over-testing" here, especially if #882 gets implemented. That said, it's not pvlib's job to act as the API's test suite so I don't mind getting rid of it if other folks feel it's unnecessary.
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.
@kanderso-nrel Is there a way to increase communication with the folks that create the API?
If people are creating their own homemade API wrappers it seems like pvlib-python's could be announced in an official capacity by the NREL team as an official way of interacting with the API. This would not only draw more users to the API and pvlib-python but hopefully spur direct communication from the API team about changes and potentially prompt their developers to maintain pvlib-python's code in direct support of the API. Could be a win-win-win for pvlib-python, NREL developers and the industry.
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.
what's the verdict here? add this two lines back or not? your call @CameronTStark. I think this is the only thing preventing a merge.
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.
Sorry for not responding -- I did send a note over to the NREL API team asking about this but haven't heard back yet. I'll share anything that comes out of that conversation.
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.
@wholmgren I added it back in. I didn't mean to take it out but it got lost in the shuffle. It's already been committed so please let me know if it's good to merge. Thanks!
Thanks @kanderso-nrel for sending a note over to the NREL API team.
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.
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.