Skip to content

rename losses.py to soiling.py #937

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 8 commits into from
Apr 7, 2020
Merged

Conversation

cwhanse
Copy link
Member

@cwhanse cwhanse commented Mar 19, 2020

Renames pvlib.losses to pvlib.soiling, functions renamed from pvlib.losses.soiling_hsu to pvlib.soiling.hsu, and pvlib.losses.soiling_kimber to pvlib.soiling.kimber

@cwhanse
Copy link
Member Author

cwhanse commented Mar 19, 2020

RTD build fail looks related to not getting forecast data from a server.

@kandersolar
Copy link
Member

The Kimber gallery example will need updates as well.

@cwhanse
Copy link
Member Author

cwhanse commented Mar 19, 2020

The Kimber gallery example will need updates as well.

Done. Do you think that caused the test failure? It was unclear to me where the failure happened.

@cwhanse
Copy link
Member Author

cwhanse commented Mar 19, 2020

This is ready for review. Open issue: deprecate this change, or just make the change?

@kandersolar
Copy link
Member

Done. Do you think that caused the test failure? It was unclear to me where the failure happened.

Building that commit locally (make html) does raise a non-zero error code without the forecast issues. Not sure if the forecast error would still have failed the build anyway if everything else was successful.

Copy link
Member

@mikofski mikofski left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks Cliff!

Copy link
Member

@wholmgren wholmgren left a comment

Choose a reason for hiding this comment

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

I think I brought this up somewhere else and apologies if I have forgotten what was decided: Do we want to deprecate losses.soiling_hsu or just make the change? kimber is new in 0.7.2 so no need to deprecate it.

@wholmgren wholmgren added the api label Mar 23, 2020
@wholmgren wholmgren added this to the 0.7.2 milestone Mar 23, 2020
@cwhanse
Copy link
Member Author

cwhanse commented Mar 24, 2020

I think I brought this up somewhere else and apologies if I have forgotten what was decided: Do we want to deprecate losses.soiling_hsu or just make the change?

My vote is not to deprecate and just move to soiling.py

@mikofski
Copy link
Member

Can I abstain? I don't want to vote

@cwhanse
Copy link
Member Author

cwhanse commented Mar 25, 2020

Can I abstain? I don't want to vote

Yes. Hearing nothing more, the "aye" has it. No deprecation on renaming losses.py.

@cwhanse
Copy link
Member Author

cwhanse commented Mar 25, 2020

@CameronTStark can you see what is going on with the py3.6 and py3.7 tests for pvlib.forecast.py? I've re-run the failed tests, same result. Maybe the data server is down, but if so I don't understand why py3.5 tests would pass.

@CameronTStark
Copy link
Contributor

CameronTStark commented Mar 26, 2020

@CameronTStark can you see what is going on with the py3.6 and py3.7 tests for pvlib.forecast.py? I've re-run the failed tests, same result. Maybe the data server is down, but if so I don't understand why py3.5 tests would pass.

The CI test failures here aren't due to anything in this PR and I suspect the failures are due to the external API.

The failures come from test_forecast.py pulling data with NAM() five times and once with GFS() with errors pertaining to TypeError: <class 'cftime._cftime.DatetimeGregorian'> is not convertible to datetime. I ran this PR locally and didn't get the same failures so this may be tricky.

I'm considering separating the --remote-data runs entirely so it can be easier to surmise if the issue is external. Edit: this would lower coverage percentage unfortunately

I'll keep an eye out for results from new PRs while digging further. Thanks for highlighting it @cwhanse!

This was referenced Mar 30, 2020
@CameronTStark CameronTStark merged commit 6b03e9d into pvlib:master Apr 7, 2020
@CameronTStark
Copy link
Contributor

Thanks @cwhanse!

@nassim86
Copy link

hey everyone . i want to print atabnle of monthly value of soiling loss as a table to get the value of each month instead of a plot in kimber model . pleas help.
thank you

@kandersolar
Copy link
Member

Hi @nassim86, we use this forum for developing the package, but your question is more about usage. Can you please repost your question to the pvlib google group instead of here? It would probably also be helpful to include more detail about what exactly you want to calculate. Thanks! https://groups.google.com/g/pvlib-python

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.

move kimber and HSU to soiling.py and remove losses.py
6 participants