Skip to content

DOC: Discuss Angstrom Kasten TL, set default alpha#563

Merged
wholmgren merged 10 commits into
pvlib:masterfrom
mikofski:discuss_angstrom_kasten_tl
Sep 7, 2018
Merged

DOC: Discuss Angstrom Kasten TL, set default alpha#563
wholmgren merged 10 commits into
pvlib:masterfrom
mikofski:discuss_angstrom_kasten_tl

Conversation

@mikofski

@mikofski mikofski commented Sep 6, 2018

Copy link
Copy Markdown
Member

pvlib python pull request guidelines

Thank you for your contribution to pvlib python! You may delete all of these instructions except for the list below.

You may submit a pull request with your code at any stage of completion.

The following items must be addressed before the code can be merged. Please don't hesitate to ask for help if you're unsure of how to accomplish any of the items below:

  • Closes add discussion and examples of Angstrom and Kasten formulas to docs #302
  • I am familiar with the contributing guidelines.
  • Fully tested. Added and/or modified tests to ensure correct behavior for all reasonable inputs. Tests (usually) must pass on the TravisCI and Appveyor testing services.
  • 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.
  • Code quality and style is sufficient. Passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • New code is fully documented. Includes sphinx/numpydoc compliant docstrings and comments in the code where necessary.
  • Pull request is nearly complete and ready for detailed review.

Brief description of the problem and proposed solution (if not already fully described in the issue linked to above):

* closes pvlib#302
* add references to Kasten 96 and Molineaux
* discuss using Kasten and calculating broadband AOD usinge Angstrom turbidity model

Signed-off-by: Mark Mikofski <bwana.marko@yahoo.com>
@mikofski

mikofski commented Sep 6, 2018

Copy link
Copy Markdown
Member Author

is this okay?

@mikofski mikofski changed the title Discuss angstrom kasten tl DOC: Discuss angstrom kasten tl Sep 6, 2018
Comment thread docs/sphinx/source/clearsky.rst Outdated
Comment thread docs/sphinx/source/clearsky.rst Outdated

In [1]: MBARS = 100 # conversion factor from mbars to Pa

In [1]: URL = 'http://rredc.nrel.gov/solar/old_data/nsrdb/1991-2005/data/tmy3/722040TYA.CSV'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There is a TMY file in pvlib/data, I think it is safer to use it rather than read from the URL.

Comment thread docs/sphinx/source/clearsky.rst Outdated
Comment thread docs/sphinx/source/clearsky.rst Outdated
@mikofski

mikofski commented Sep 6, 2018

Copy link
Copy Markdown
Member Author

Great feedback, thanks. I agree with all comments.

FYI: There are no italics in the IPython example, that's just GitHub not rendering correctly b/c it doesn't understand the .. ipython:: directive, okay?

@mikofski

mikofski commented Sep 6, 2018

Copy link
Copy Markdown
Member Author

@wholmgren is there a built version of the docs that I can point reviewers to online?

@wholmgren

Copy link
Copy Markdown
Member

is there a built version of the docs that I can point reviewers to online?

You'd need to login to readthedocs.org and configure it to build this particular branch of your fork. It only takes a few minutes to set up the first time.

@wholmgren wholmgren added this to the 0.6.0 milestone Sep 6, 2018

@wholmgren wholmgren left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

thanks mark. minor edits suggested.

Comment thread docs/sphinx/source/clearsky.rst Outdated
Comment thread docs/sphinx/source/clearsky.rst Outdated

.. ipython::

In [1]: MBARS = 100 # conversion factor from mbars to Pa

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I know you have your reasons for using caps here, but I think it's better for most readers to see lower case.

Comment thread docs/sphinx/source/clearsky.rst Outdated
Comment thread docs/sphinx/source/clearsky.rst Outdated
Solis model requires AOD at 700 nm. Models exist to convert AOD between
different wavelengths, as well as convert Linke turbidity to AOD and PW
[Ine08con]_, [Ine16]_.
Solis model requires AOD at 700 nm. The function,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

delete The function and commas. I think the formatting makes it clear that it's a function. But you can keep it if you prefer this way.

Comment thread docs/sphinx/source/clearsky.rst Outdated
Solis model requires AOD at 700 nm. The function,
:py:func:`~pvlib.atmosphere.angstrom_aod_at_lambda`, is useful for converting
AOD between different wavelengths using the Angstrom turbidity model and given
the Angstrom exponent, :math:`\alpha`, which can be calculated from AOD at two

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

please split this into 2 or more sentences.

Comment thread docs/sphinx/source/clearsky.rst Outdated
AOD between different wavelengths using the Angstrom turbidity model and given
the Angstrom exponent, :math:`\alpha`, which can be calculated from AOD at two
wavelengths with the :py:func:`~pvlib.atmosphere.angstrom_alpha` function. The
function, :py:func:`~pvlib.atmosphere.bird_hulstrom80_aod_bb`, can be used to

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

same comment on "the function" as above

* use TMY in pvlib/data, lowercase MBARS, use non-specific variable names
* split output from readtmy3 instead of using indexing for clarity
@stickler-ci

stickler-ci Bot commented Sep 6, 2018

Copy link
Copy Markdown

I couldn't find a .stickler.yml file in this repository. I can make one for you, or you can create one by following the documentation.

@mikofski

mikofski commented Sep 6, 2018

Copy link
Copy Markdown
Member Author

FYI: rtfd build of my fork is failing and I don't know why

@mikofski

mikofski commented Sep 6, 2018

Copy link
Copy Markdown
Member Author

Also there is currently no default for alpha in pvlib.atmosphere.angstrom_aod_at_lambda, but I think we should set it at 1.14 as recommended by NREL SPECTRL2 (see section 2)

When a single value of α is used to represent the rural aerosol model, the value should be α = 1.140.

It's also in the source for the origin FORTRAN code

	C   ALPHA= POWER ON ANGSTRON TURBIDITY EXPRESSION (1.14 FOR RURAL)

And in both spreadsheet implementation and C-headers, see here.

@wholmgren

Copy link
Copy Markdown
Member

@mikofski I'm not sure either. I've never seen that error. The common errors I see are intermittent and related to timeouts of one thing or another. I poked around the settings a little bit but nothing jumped out. Sorry.

@wholmgren

Copy link
Copy Markdown
Member

Fine with me if you want to make a default alpha. If so, please update whatsnew and the PR title accordingly.

@mikofski mikofski changed the title DOC: Discuss angstrom kasten tl DOC: Discuss Angstrom Kasten TL, set default alpha Sep 6, 2018

@wholmgren wholmgren left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I rendered the documentation and it mostly looks good.

Comment thread docs/sphinx/source/clearsky.rst Outdated
...: altitude=tmy_header['altitude'], pressure=tmy_data['Pressure']*mbars,
...: temperature=tmy_data['DryBulb'])

In [1]: am_rel = atmosphere.relativeairmass(solpos.apparent_zenith)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

get_relative_airmass

Comment thread docs/sphinx/source/clearsky.rst Outdated

In [1]: am_rel = atmosphere.relativeairmass(solpos.apparent_zenith)

In [1]: am_abs = atmosphere.absoluteairmass(am_rel, tmy_data['Pressure']*mbars)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

get_absolute_airmass

Comment thread docs/sphinx/source/clearsky.rst Outdated
In [1]: tmy_data, tmy_header = tmy.readtmy3(tmy_file) # read TMY data

In [1]: dt = pd.DatetimeIndex(start='1999-01-01 00:00:00', end='1999-12-31 23:59:59',
...: freq='H')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

out of curiosity why do this instead of use the readtmy3 coerce_year kwarg?

Comment thread docs/sphinx/source/clearsky.rst Outdated
the :py:func:`pvlib.clearsky.ineichen` function.


The :py:func:`~pvlib.atmosphere.kasten96_lt` function can be used to calculate

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The "Turbidity data" section below introduces the clearsky.lookup_linke_turbidity function. Your example uses this function, so I think your new text/example should be appended to that section. Am I missing something?


In [1]: alpha_exponent = atmosphere.angstrom_alpha(aod1240nm, 1240, aod550nm, 550)

In [1]: aod700nm = atmosphere.angstrom_aod_at_lambda(aod1240nm, 1240, alpha_exponent, 700)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

suggest a line(s) that prints the alpha_exponent and aod700nm

@wholmgren wholmgren merged commit ec329e2 into pvlib:master Sep 7, 2018
@mikofski mikofski deleted the discuss_angstrom_kasten_tl branch September 8, 2018 17:45
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.

3 participants