Skip to content
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

Add option to control airmass/perez enhancement factor in clearsky.ineichen #459

Merged
merged 18 commits into from Aug 6, 2018

Conversation

wholmgren
Copy link
Member

@wholmgren wholmgren commented May 14, 2018

See #435 for context.

  • Closes Artefact in Ineichen clearsky model for sun close to horizon #435
  • Fully tested. Added and/or modified tests to ensure correct behavior for all reasonable inputs. Tests must pass on the TravisCI and Appveyor testing services.
  • Code quality and style is sufficient. Passes git diff upstream/master -u -- "*.py" | flake8 --diff and/or landscape.io linting service.
  • New code is fully documented. Includes sphinx/numpydoc compliant docstrings and comments in the code where necessary.
  • Updates entries to docs/sphinx/source/api.rst for API changes.
  • Adds description and name entries in the appropriate docs/sphinx/source/whatsnew file for all changes.

Examples of ineichen with and without the enhancement at a few latitudes, with simplied solis as a reference:

ineichen_perez_32 0
ineichen_perez_60 0
ineichen_perez_70 0

While the differences at higher latitudes are more noticeable, the differences at lower latitudes are still notable: a few % difference at solar noon, unphysical pedestals near the horizon.

Many tests failed when I changed the default, so still work to do on that.

@wholmgren
Copy link
Member Author

Some test failures are due to #481.

@wholmgren
Copy link
Member Author

wholmgren commented Jun 25, 2018

This PR is going to end up accomplishing most of the remaining work for #394.

To do includes:

@wholmgren
Copy link
Member Author

wholmgren commented Aug 1, 2018

I think this is close to ready to go. It needs someone to review it, especially the changes to clearsky.py and test_clearsky.py.

The majority of the other changes are cleaning up the test suite so that it's not tightly coupled to this function. The perez_enhancement change made all of those test functions fail so I think we had no choice but to address the broader changes here.

The detect_clearsky failures were caused by the scaling factor, alpha, changing in response to the ~1% change in ghi returned by the ineichen function. I adjusted the expected test values to get them to pass.

to do:

  • add mock to min and py2 test environments
  • fix failing test_location tests in py 3.4, 3.5 (syntax issue with mock)

@adriesse
Copy link
Member

adriesse commented Aug 1, 2018

For what it's worth, I browsed through the changes and didn't notice any problems.

Is it possible to have tests that compare outputs to the previous stable release, instead of hard coding lists of constants?

@wholmgren
Copy link
Member Author

wholmgren commented Aug 1, 2018

Thanks @adriesse. Interesting idea to compare test results to previous stable releases. Maybe there is a framework/package for that?

Copy link
Member

@cwhanse cwhanse left a comment

Choose a reason for hiding this comment

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

In clearsky.py:

I recommend we remove the comment paragraphs at line 88 starting with "Dan's note..." and "Create the corrected TL..." This was developmental material in PVLib which was not included in the release.

We should revise the paragraph at line 104 to point to the Ineichen / Perez paper [1] as the reference. We should clarify that [2] implements a different version of the model in [1]. Suggest the following:

# ghi is calculated using either the equations in [1] by setting perez_enhancement=False (default behavior) or using the model in [2] by setting perez_enhancement=True.

@wholmgren
Copy link
Member Author

@cwhanse I changed the comments. I'm not 100% sure I captured what you were going for.

I think this is ready to merge. The test pass on all Travis CI configurations. The appveyor test failures are intermittent and unrelated.

@cwhanse
Copy link
Member

cwhanse commented Aug 2, 2018

I think the comments are fine. My intent was to discard the no-longer relevant text from the Matlab file, and to clarify how this code implements the equations in those two Ineichen/Perez papers.

Any comments from others before this is merged?

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.

The changes to ineichen look good to me, I briefly glanced at some of the tests and mocker stuff, but not thoroughly. Thanks for making these changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants