Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Add hint that cec-database does not provide coefficients for the sapm function #123

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Feb 18, 2016
Merged

Add hint that cec-database does not provide coefficients for the sapm function #123

merged 5 commits into from
Feb 18, 2016

Conversation

uvchik
Copy link
Contributor

@uvchik uvchik commented Feb 18, 2016

Please have a look at the commits 5f0df9f and 110688e. Commit f81529b contains only pep8 changes. Sorry it is knee-jerk and if you don't like you can just revert it.

The first commit (5f0df9f) fixes the table in the docstring of the sapm function. It does not show up in the current documentation (see: http://pvlib-python.readthedocs.org/en/latest/pvlib.html#pvlib.pvsystem.sapm)

The third commit (110688e) adds the new hints. Please fix typos if necessary, my English is not too good.

There are 6 Errors using nosetest3 but they have been there before my changes.

@uvchik uvchik mentioned this pull request Feb 18, 2016
Closed
@uvchik
Copy link
Contributor Author

uvchik commented Feb 18, 2016

It has nothing to do with this pull request but as I compiled the documentation I fixed this warning.

wholmgren
wholmgren reviewed Feb 18, 2016
View reviewed changes
pvlib/pvsystem.py
Remark that modules of the Sandia module database contain these
coefficients but modules of the CEC module database do not. Both databases
can be accessed using :py:func:`retrieve_sam`.

Copy link
Member

Choose a reason for hiding this comment

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

Minor suggested edit...

The modules in the Sandia module database contain these coefficients, but the modules in the CEC module database do not.

Or you could replace "Remark that" with something like "We note that".

@wholmgren
Copy link
Member

Thanks, looks great! I'd been meaning to fix those documentation warnings too.

I'll add this to the whatsnew file after we merge #93.

What are the nosetest3 errors? The travis build passed and the appveyor build fails due to an issue with continuum's new mkl libraries (I think).

@uvchik
Copy link
Contributor Author

uvchik commented Feb 18, 2016

The reason why AppVeyor does not pass has to do with failing tests:

pvlib.test.test_atmosphere.test_absoluteairmass_numeric ... ok
pvlib.test.test_atmosphere.test_absoluteairmass_nan ... ok
pvlib.test.test_clearsky.test_ineichen_required ... Command exited with code -1073741819

It might has to do something with a missing scipy installation on my computer. Here is the output of nosetests (the outputs of nosetests and nosetest3 are the same).

======================================================================
ERROR: pvlib.test.test_clearsky.test_ineichen_required
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/local/lib/python3.4/dist-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/home/uwe/rli-lokal/git_home/pvlib-python/pvlib/test/test_clearsky.py", line 37, in test_ineichen_required
    out = clearsky.ineichen(times, tus)
  File "/home/uwe/rli-lokal/git_home/pvlib-python/pvlib/clearsky.py", line 120, in ineichen
    interp_turbidity=interp_turbidity)
  File "/home/uwe/rli-lokal/git_home/pvlib-python/pvlib/clearsky.py", line 230, in lookup_linke_turbidity
    'supply your own turbidities.')
nose.proxy.ImportError: The Linke turbidity lookup table requires scipy. You can still use clearsky.ineichen if you supply your own turbidities.
-------------------- >> begin captured logging << --------------------
pvlib: DEBUG: irradiance.extraradiation()
pvlib: DEBUG: Calculating ET rad using Spencer method
pvlib: DEBUG: using PyEphem to calculate solar position
pvlib: DEBUG: tz_localize to US/Arizona and then tz_convert to UTC
--------------------- >> end captured logging << ---------------------

But why does appveyor has the same problem?
https://ci.appveyor.com/project/wholmgren/pvlib-python-fv2to/build/1.0.77/job/840xrjrxb1jnben7

It is just a python2 problem.
https://ci.appveyor.com/project/wholmgren/pvlib-python-fv2to

@wholmgren
Copy link
Member

Yes, on your computer it's due to missing scipy. I added the @requires_scipy decorator to that test in #93 so that developers can still run most of the tests without scipy.

I think that the issue on appveyor is a little different. That error is the result of something going very wrong when the python interpreter interacts with the scipy binary that Continuum created. While I don't know exactly where the problem is, this would not be the first time that Continuum distributed a less than perfect scipy distribution.

wholmgren added a commit that referenced this pull request Feb 18, 2016
@wholmgren
Merge pull request #123 from uvchik/master
b7d8e64 
Add hint that cec-database does not provide coefficients for the sapm function
@wholmgren wholmgren merged commit b7d8e64 into pvlib:master Feb 18, 2016
@wholmgren
Copy link
Member

Thanks again!

@wholmgren wholmgren added this to the 0.3 milestone Mar 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
documentation
Projects
None yet
Milestone
0.3.0
Development

Successfully merging this pull request may close these issues.
2 participants
@uvchik @wholmgren
Morty Proxy This is a proxified and sanitized view of the page, visit original site.