-
Notifications
You must be signed in to change notification settings - Fork 1.1k
add linke turbidity formulas module to atmosphere #278
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
Conversation
* add kasten 1996 pyrheliometric formula for linke turbidity factor * move atmosphere into package, just for fun, why not? * make __init__ with same api as before Signed-off-by: Mark Mikofski <[email protected]>
* append linke_turb_forms.py to atmosphere.py * rm -rf atmosphere/ Signed-off-by: Mark Mikofski <[email protected]>
* import xrange from six for python 3 compatibility * fix pyrheliometric formula is from 1980
* just use `range` instead of importing `six` * change function to `kasten80_lt` to match `gueymard94_pw` * start adding references
* add lots of references * cite text quoted directly from Ineichen * acknowledge Armel * fix docstrings for numpydocs
I think this is ready. The Travis failure is not related to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mikofski.
Is there any way to simplify the API? It feels very complicated to me. Also, the various types of potential inputs are not tested.
Please also add the functions to the api.rst file and update whatsnew.
pvlib/atmosphere.py
Outdated
|
||
Method can be either ``'Molineaux'`` or ``'Bird-Huldstrom'``. Airmass less | ||
than 1 or greater than 6 will return ``NaN``. Precipitable water less than | ||
zero or greater than 5[cm] will also return ``NaN``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear to me that we need to set these limits. Garbage in, garbage out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, removed filters and put these limits as a .. warning::
pvlib/atmosphere.py
Outdated
than 1 or greater than 6 will return ``NaN``. Precipitable water less than | ||
zero or greater than 5[cm] will also return ``NaN``. | ||
|
||
Based on originam implementation by Armel Oumbe. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
original
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅
pvlib/atmosphere.py
Outdated
Linke turbidity | ||
|
||
References | ||
---------- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are a handful of typos in the references
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1-3 good references might be more useful than 8.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed the references errors. Not sure I agree about the references. It's hard for me to tease out all of these pieces from just one or two references. I suppose we could just use the Ineichen references which include almost all of the other ones, although some are incorrect:
- "A new airmass independent ..." (2002) includes: Kasten-1980, Kasten-1996, Linke-1922, Molineaux-1989
- "Conversion function between Linke ..." (2008) includes: Berk-1989,1996, Bird-1980 (incorrect reference), Mueller and Mayer
What is your preference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok with a longer references section if that's your preference.
pvlib/atmosphere.py
Outdated
if method == 'Molineaux': | ||
# get aod at 700[nm] from alpha for Molineaux (1998) | ||
delta_a = angstrom_aod_at_lambda(aod, alpha) | ||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably best to elif method == 'bird-hulstrom'
and then else: raise ValueError('invalid method')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅
pvlib/atmosphere.py
Outdated
Angstrom :math:`\\alpha` exponent corresponding to wavelength in ``aod`` | ||
lambda0 : numeric | ||
desired wavelength in nanometers, defaults to 700[nm] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need a Returns section
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅
pvlib/atmosphere.py
Outdated
return lt | ||
|
||
|
||
def angstrom_aod_at_lambda(aod, alpha, lambda0=700.0): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function needs its own tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅
pvlib/test/test_atmosphere.py
Outdated
atmos_path = os.path.dirname(os.path.abspath(__file__)) | ||
pvlib_path = os.path.dirname(atmos_path) | ||
melbourne_fl = tmy.readtmy3(os.path.join( | ||
pvlib_path, 'data', '722040TYA.CSV') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why add a new tmy file instead of using the existing sample file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no particular reason, I like that Melbourne-FL site, it's close to Cocoa where I have irradiance data. also although no site matches the Linke turbidity data well, the Alaska site has bigger differences, and also bigger differences between broadband approximations relative to Melbourne-FL. Maybe these are not good tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to define ~2-5 element arrays that span the range of allowed input values, and also define the expected return values. I can't quickly inspect the values that are being tested and expected in this test. Maybe these values span the space well enough?
am = np.array([1, 3, 5])
pw = np.array([0, 2.5, 5])
aod700 = np.array([0, 0.1, 1])
lt_molineaux_expected = np.array('your precalculated values')
lt_bird_hulstrom_expected = np.array('your precalculated values')
...
np.assert_allclose(lt_molineaux_expected, lt_molineaux)
np.assert_allclose(lt_bird_hulstrom_expected, lt_bird_hulstrom)
Then you don't need to read a tmy file or lookup linke turbidity in the test. Seems more simple and more explicit to me. I'm not sure how much I trust the tmy and linke turbidity files anyways.
@wholmgren great feedback on all points, thanks! 👍👍 |
* remove alpha argument * update docstring to explain that if molineaux method, then aod is shape is only aod at 700[nm] but if bird-hulstrom, then aod should be both aod380 and aod500 * add see also to angstrom_aod_at_lambda * add warning for air mass or pwat out of range * check for bird-hulstrom, raise type error if neither molineaux nor * also simplify angstrom_aod_at_lambda() api, change aod to aod0, change lambda0 to lambda1, and add lambda0, update docstring * update test_atmosphere.test_kasten96_lt with new changes
* separate arguments so their shapes don't change depending on method, ie: before aod could be 1, 2, N, N x 1 or N x 2 depending on whether Molineaux or Bird-Hulstrom were used, now, there are 3 separate args, aod700, aod380 and aod500. * fix misspelling of Hulstrom, no "d", note several sources also make this error :( * add new methods to api.rst * add test_angstrom tests * add raises value error test for bad method * add lower() to compare only lower case of method name
* fix more hulstrom errors, no "d" * fix typos in referecnes
ready for your review:
|
Thanks. Maybe tonight. Definitely by the end of the week.
…On Tue, Jan 31, 2017 at 2:47 PM Mark Mikofski ***@***.***> wrote:
ready for your review:
- simplified kasten96_lt API
- added tests for angstrom methods
- added test for bad method raises ValueError
- add new methods to api.rst
- update what's new 0.4.4
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#278 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AELiR4eVqvUhj0fooMmIYKi1j0qgUxcFks5rX6vqgaJpZM4LDR0Z>
.
|
These are great changes, thanks! landscape is complaining about a few things related to the doc strings. |
The other landscape errors are due to the DOI external links which are too long. Can we add doi to
|
Sorry, didn't look carefully enough. All sounds good. |
* add 'doi': ('http://dx/doi.org/%s', 'DOI: ') to extlink in conf.py * change doi links everywhere to use :doi:`doi number` instead of links
* instead of """docstring with :math:`\\alpha` * add Co-Action publishing for original Tellus A DOI before moving to Taylor & Francis
Okay, I think it's ready now. I added doi as an extlink to the conf.py and I change the docstring to use r"""docstring with :math: |
Okay, that was my last change. Sorry! 😉 I really think it's ready now. Let me know if there are any more comments or suggestions. |
For the remaining landscape complaint:
That's the simplified API for the Kasten LT formula. I had some ideas about that but I wasn't what was best. Basically one idea would be to just have one AOD input called def bird_hulstrom80_aod_bb(aod380, aod500):
"""Calculate broadband AOD using Bird-Hulstrom approximation"""
# put Bird-Hulstrom references here
return 0.27583 * aod380 + 0.35 * aod500
def kasten96_lt(am, pwat, aod_bb):
"""
Calculate Linke turbidity factor using Kasten pyrheliometric formula.
.. warning::
These calculations are only valid for air mass less than 5[atm] and
precipitable water less than 5[cm].
Parameters
----------
am : numeric
airmass, pressure corrected in atmospheres
pwat : numeric
precipitable water or total column water vapor in centimeters
aod_bb : numeric
broadband AOD or AOD measured at 700[nm]
Returns
-------
Linke turbidity
See also
--------
bird_hulstrom80_aod_bb
angstrom_aod_at_lambda
"""
# remove Bird-Hulstrom references
delta_cda = -0.101 + 0.235 * am ** (-0.16) # rayleigh "clean dry atmosphere"
delta_w = 0.112 * am ** (-0.55) * pwat ** (0.34) # water vapor
delta_a = aod_bb
return -(9.4 + 0.9 * am) * np.log(np.exp(-am * (delta_cda + delta_w + delta_a))) / am Would you prefer this approach? Let me know and I'll make the change :) |
Nice. I really like the simplicity of your latest proposal. I'm guessing that most users would find it easier to use. How about this for alternative For consistency with existing functions, we'd want to use What did you think about simplifying the tests by using a couple of hardcoded values instead of a TMY file? It's definitely not necessary for this PR, but it would be nice to have a brief section in clearsky.rst (or maybe a new atmosphere.rst) with a few examples and a little bit of discussion. Something for the future. |
Sounds good. I'll make the changes and get back to you. |
* change kasten96_lt AOD args to just a single arg called aod_bb for broadband AOD * remove "method" arg and if-then-else on method.lower() in kasten96_lt() * add note that according to Molineaux, aod at 700 nm is a good approximation for broadband aod * add new function bird_hulstrom80_aod_bb to atmosphere.py * add it to api.rst * add new test for bird_hulstrom80_aod_bb() to test_atmosphere.py * update test_kasten96_lt() to use aod_bb * use verbose names: - s/am/airmass_absolute/g - s/pwat/precipitable_water/g * fix angstrom aod test to use more realistic values * don't use brackets around units * remove test for raises ValueError in test_kasten
almost there, just cleaning up the data for the test, and removing the TMY file, should be ready soon. |
* values span pwat range 0 - 5 cm, and airmass between 1 & 2.5 atm * remove tmy3 file * don't try Bird-Hulstrom, not necessary * make pylint happy, isclose(a, b) a & b should be array or iterable
It's ready. 🚀 I made a separate issue for the docs. 📚 I feel like it may require some iterations, so would rather split into a new PR, OK? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will merge as soon as the asserts are fixed
pvlib/test/test_atmosphere.py
Outdated
aod550 = 0.15 | ||
aod1240 = 0.05 | ||
alpha = atmosphere.angstrom_alpha(aod550, 550, aod1240, 1240) | ||
np.isclose(alpha, [1.3513924317859232]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to assert np.allclose or use np.testing.assert_allclose
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird. np.isclose
should work since these are scalars. Pylint told me to change the scalar to an intake and it passes tests fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*iterable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed these back to scalars, although pylint now complains, oh well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, it's pylint that's the trouble:
In [4]: np.isclose(2, [2])
Out[4]: array([ True], dtype=bool)
In [5]: np.isclose(2, 2)
Out[5]: True
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's that you have to assert something is true or not. np.isclose only returns an array.
pvlib/test/test_atmosphere.py
Outdated
alpha = atmosphere.angstrom_alpha(aod550, 550, aod1240, 1240) | ||
np.isclose(alpha, [1.3513924317859232]) | ||
aod700 = atmosphere.angstrom_aod_at_lambda(aod550, 550, alpha) | ||
np.isclose(aod700, [0.10828110997681031]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same assert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
pvlib/test/test_atmosphere.py
Outdated
"""Test Bird_Hulstrom broadband AOD.""" | ||
aod380, aod500 = 0.22072480948195175, 0.1614279181106312 | ||
bird_hulstrom = atmosphere.bird_hulstrom80_aod_bb(aod380, aod500) | ||
np.isclose([0.09823143641608373], bird_hulstrom) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same assert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
pvlib/test/test_atmosphere.py
Outdated
def test_kasten96_lt(): | ||
"""Test Linke turbidity factor calculated from AOD, Pwat and AM""" | ||
timestamps = pd.DatetimeIndex(MELBOURNE_FL[0]) | ||
timestamps = timestamps.tz_localize('UTC').tz_convert('Etc/GMT+5') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one more that I just noticed... this line errors on the python27-min configuration because timestamps
is already tz aware: https://travis-ci.org/pvlib/pvlib-python/jobs/198282511
I don't understand why the python-36 build completed, but we should still fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was not tz aware on my machine, it gets converted to a naive date time index in utc, that's why I had to localize as UTC then convert to us/Eastern. Is this a pandas or pytz version issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is what I get with pandas
In [21]: pd.DatetimeIndex(MELBOURNE_FL[0])
Out[21]:
DatetimeIndex(['1999-01-31 17:00:00', '2000-02-20 20:00:00',
'2000-02-22 18:00:00', '2000-02-24 20:00:00',
'1995-03-02 19:00:00', '1995-03-11 17:00:00',
'1995-03-12 18:00:00', '1995-03-20 16:00:00',
'1995-03-20 19:00:00', '1995-03-22 16:00:00',
'1995-03-22 19:00:00', '1995-04-07 14:00:00',
'1995-04-10 14:00:00', '1995-04-21 14:00:00',
'2004-05-01 13:00:00', '2004-05-03 13:00:00',
'2004-05-05 18:00:00', '2004-05-16 14:00:00',
'2004-05-21 20:00:00', '2004-05-24 16:00:00',
'2004-05-31 14:00:00', '2002-06-04 21:00:00',
'2002-06-16 22:00:00', '2002-06-17 13:00:00',
'2000-07-01 13:00:00', '2000-07-05 12:00:00',
'2000-07-06 12:00:00', '2000-07-06 22:00:00',
'2000-07-25 20:00:00', '2001-10-14 18:00:00',
'2001-10-15 16:00:00', '2003-11-04 19:00:00',
'1999-12-20 18:00:00'],
dtype='datetime64[ns]', freq=None)
In [22]: pd.__version__
Out[22]: u'0.19.1'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the conda list
command in the travis builds reports pytz 2016.10 for both builds, so maybe it's pandas. I couldn't find anything about a change in the pandas behavior here, so maybe there's a bug in the newer version. strange.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no change in newest pandas or pytz
In [3]: pd.__version__
Out[3]: u'0.19.2'
In [4]: pd.DatetimeIndex(MELBOURNE_FL[0])
Out[4]:
DatetimeIndex(['1999-01-31 17:00:00', '2000-02-20 20:00:00',
'2000-02-22 18:00:00', '2000-02-24 20:00:00',
'1995-03-02 19:00:00', '1995-03-11 17:00:00',
'1995-03-12 18:00:00', '1995-03-20 16:00:00',
'1995-03-20 19:00:00', '1995-03-22 16:00:00',
'1995-03-22 19:00:00', '1995-04-07 14:00:00',
'1995-04-10 14:00:00', '1995-04-21 14:00:00',
'2004-05-01 13:00:00', '2004-05-03 13:00:00',
'2004-05-05 18:00:00', '2004-05-16 14:00:00',
'2004-05-21 20:00:00', '2004-05-24 16:00:00',
'2004-05-31 14:00:00', '2002-06-04 21:00:00',
'2002-06-16 22:00:00', '2002-06-17 13:00:00',
'2000-07-01 13:00:00', '2000-07-05 12:00:00',
'2000-07-06 12:00:00', '2000-07-06 22:00:00',
'2000-07-25 20:00:00', '2001-10-14 18:00:00',
'2001-10-15 16:00:00', '2003-11-04 19:00:00',
'1999-12-20 18:00:00'],
dtype='datetime64[ns]', freq=None)
In [6]: pytz.__version__
Out[6]: '2016.10'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get the same thing on pandas 0.19.2. But also this...
In [11]: pd.Timestamp(MELBOURNE_FL[0][0])
Out[11]: Timestamp('1999-01-31 12:00:00-0500', tz='pytz.FixedOffset(-300)')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, I didn't like this test and want to change it so this gives me the chance. |
* fix assert np.isclose() should be scalar only * super simplify test_kasten96_lt() to ranges of inputs and expected outputs, don't need to compare to actual data, just test that method is working nominally
let's see how this goes. sorry for all the iterations. |
pvlib/test/test_atmosphere.py
Outdated
|
||
|
||
def test_angstrom_aod(): | ||
"""Test Angstrom turbidity model functions.""" | ||
aod550 = 0.15 | ||
aod1240 = 0.05 | ||
alpha = atmosphere.angstrom_alpha(aod550, 550, aod1240, 1240) | ||
np.isclose(alpha, [1.3513924317859232]) | ||
np.isclose(alpha, 1.3513924317859232) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry for not being clear. this line and others like it should read assert np.isclose(alpha, 1.3513924317859232)
no worries. it would help if the "so called judge" (the test suite) actually worked reliably. |
Landscape to the rescue - cleaned up unused import ( |
@mikofski In case my comment was buried in the exchange on Saturday morning... the functions that use I'll also close #101 when this is merged. |
* remove old melbourne-fl test data
Oops! Sorry about that! Hopefully good to go. |
Great, thanks Mark! |
Signed-off-by: Mark Mikofski [email protected]