Skip to content

Linke turbidity factor fixes #264

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
Nov 18, 2016
Merged

Conversation

mikofski
Copy link
Member

fixes #263 and fixes #262

  • shift outputmin to zero, since (1) zero based indexing, and (2) documentation states that indices of LT data are 0.0833[arcdeg] apart
  • shift from edge to center
  • raise IndexError for out of bound lat/lon
  • add test_linke_turbidity_corners() to test corners are not out of bounds, PASSES, but older tests fail since increment is different.

* set minimum index as 0 not 1
* adjust _linearly_scale so that is increments are 5 minutes or 1/12 of an
 arcdegree from the centers of neighboring indices
* this fixes the out of bounds error for latitudes or longitudes at the
 limits of the range
* move _linearly_scale to be right after lookup_linke_turbidity
* add link to pdf on AOD and LT from MeteoTest
* raise IndexError if outputmatrix is more than half an index outside of
 the limits

Signed-off-by: Mark Mikofski <[email protected]>
Signed-off-by: Mark Mikofski <[email protected]>
@wholmgren
Copy link
Member

Thanks for digging into this!

I'm a bit skeptical that the new log messages are going to be useful to anyone after this is merged. I've been slowly removing all of the silly logging that I added before I was using pdb.

You can get rid of that todo comment, as far as I'm concerned. I think users have a responsibility to follow our conventions.

The tests that throw IndexErrors should use pytest as shown here: http://doc.pytest.org/en/latest/assert.html#assertions-about-expected-exceptions

@mikofski
Copy link
Member Author

@wholmgren do you want me to revise the 3 failing tests with new expected values?

pvlib/test/test_clearsky.py::test_lookup_linke_turbidity FAILED
pvlib/test/test_clearsky.py::test_lookup_linke_turbidity_months FAILED
pvlib/test/test_clearsky.py::test_lookup_linke_turbidity_nointerp_months FAILED

@mikofski
Copy link
Member Author

mikofski commented Nov 16, 2016

Reverse engineering the current expected LT values to make sure the new calculation is consistent:

test_lookup_linke_turbidity()

lat, lon = 32.2, -111
# using _linearly_scale
lat = (lat - 90.) * (-2159. / 180.) + 1.  # 694.2788888888888
lon = (lon - (-180.)) * (4319. / 360.) + 1.  # 828.8083333333333
# now use new lookup method, shifting the indices to their corrected inputs at (32.125, -110.875)
clearsky.lookup_linke_turbidity(
    times, 90. - np.around(lat) / 12. - (1. / 24.), np.around(lon) / 12. - 180. + (1. / 24.)
)
# 2014-06-24 00:00:00-07:00    3.101266
# 2014-06-24 12:00:00-07:00    3.101266
# 2014-06-25 00:00:00-07:00    3.114430
# Freq: 12H, dtype: float64

yields the same result as the expected answer

Signed-off-by: Mark Mikofski <[email protected]>
@wholmgren
Copy link
Member

Can you look into doing the same thing for the rest of the tests? I'm guessing that it will work for some but not others. Maybe can adjust the precision of the tests too.

@wholmgren wholmgren added the bug label Nov 16, 2016
@wholmgren wholmgren added this to the 0.4.2 milestone Nov 16, 2016
@mikofski
Copy link
Member Author

Can you tell me which tests aren't passing? All test_clearsky.py pass for me.

* use calendar month days instead of approximation of monthly middles
* also account for leap years
* if all years are leap years, or not any are leap years, use fast interp,
 otherwise loop and interpolate each timestamp
* add _leapyear tests and update expected values for new interpolated
 Linke turbidity factors

Signed-off-by: Mark Mikofski <[email protected]>
@wholmgren
Copy link
Member

There is a failing test in test_location and a bunch in test_modelchain

https://travis-ci.org/pvlib/pvlib-python/jobs/176473332

Other than copy/pasting a bunch of new values, the easiest way to fix the modelchain tests might be to manually supply the irradiance data in the mc.run_model calls. Is that asking too much?

@mikofski
Copy link
Member Author

There is an error in test_forecast.test_process_data[NDFD] that I don't think has anything to do with Linke turbidity factors, but I might be wrong?

==================================== ERRORS ====================================
__________________ ERROR at setup of test_process_data[NDFD] ___________________
request = <SubRequest 'model' for <Function 'test_process_data[NDFD]'>>
    @requires_siphon
    @pytest.fixture(scope='module', params=_modelclasses)
    def model(request):
        amodel = request.param()
        amodel.raw_data = \
>           amodel.get_data(_latitude, _longitude, _start, _end)
pvlib/test/test_forecast.py:51: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
pvlib/forecast.py:252: in get_data
    self.data = self._netcdf2pandas(self.netcdf_data, self.query_variables)
pvlib/forecast.py:333: in _netcdf2pandas
    self.set_time(netcdf_data.variables[time_var])
pvlib/forecast.py:357: in set_time
    times = num2date(time[:].squeeze(), time.units)
netCDF4/_netCDF4.pyx:3695: in netCDF4._netCDF4.Variable.__getitem__ (netCDF4/_netCDF4.c:37923)
    ???
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
>   ???
E   IndexError
netCDF4/_netCDF4.pyx:4374: IndexError

@wholmgren
Copy link
Member

Awesome, thanks! Two minor things left to do:

  1. add @requires_scipy to your new corner cases test. That's the reason for the failure on the travis py27-min build.
  2. add some notes to the 0.4.2 whatsnew file. Be sure to mention that typical modeling results could change by ~1%, depending on location, if they depend on the turbidity table.

The test_forecast error is unrelated (sorry).

@wholmgren wholmgren merged commit 300d1ab into pvlib:master Nov 18, 2016
@wholmgren
Copy link
Member

Thanks @mikofski!

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.

linke turbidity resolution should be 1/12 of an arcdegree Linke turbidity out of bounds error at (90, 180)
2 participants