Skip to content

Handle overflow condition in pvlib.clearsky.haurwitz #364

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 9 commits into from
Aug 15, 2017
Merged

Handle overflow condition in pvlib.clearsky.haurwitz #364

merged 9 commits into from
Aug 15, 2017

Conversation

cwhanse
Copy link
Member

@cwhanse cwhanse commented Aug 12, 2017

Revised test to operate on zenith rather than time, to remove dependence on solarposition

Handles overflow when apparent zenith is slight greater than 90. Revised
test case to operate on zenith rather than time, removes dependence on
solarposition.
Resolve errors in test definition
Remove use of np.where in haurwitz, fix declaration of pandas objects
Changed np arrays from 2d to 1d for use as dataframe indices
Resolving type conflict inside haurwitz function for casting output as a
dataframe
@cwhanse
Copy link
Member Author

cwhanse commented Aug 12, 2017

Ready for review

@wholmgren
Copy link
Member

Thanks, Cliff. Looks good, I only have a couple of small style nitpicks...

The line with the calculation is too long (> 79 characters).

No spaces around equal signs inside a function call, so change data = clearsky_ghi to data=clearsky_ghi.

Please delete the old, commented test code from the test file.

Can you create a new docs/sphinx/source/whatsnew/v0.5.1.rst file with a note about the change?

I see that there is a preexisting mistake in the function's documentation. The documentation claims that the function will return a Series, however, it always returns a DataFrame (I think this was done to be consistent with other clear sky functions). Can you go ahead and fix this?

I see that you first tried to use np.where instead of array indexing. Why did you switch? I'm guessing that np.where has better behavior for scalar inputs and it should preserve nans. It might also be faster because it only calculates the mask once and it avoids indexing. I think it's also fine the way it is.

@cwhanse
Copy link
Member Author

cwhanse commented Aug 14, 2017 via email

Fix line length, formatting and documentation in clearsky.haurwitz.
Create whatsnew\v0.5.1.rst. Delete commented old code from
test\test_clearsky.py
@wholmgren wholmgren merged commit 39c0862 into pvlib:master Aug 15, 2017
@wholmgren
Copy link
Member

Thanks, Cliff!

@cwhanse cwhanse deleted the #363 branch September 18, 2017 15:47
@wholmgren wholmgren added this to the 0.5.1 milestone Oct 17, 2017
@wholmgren wholmgren added the bug label Oct 17, 2017
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.

2 participants