Skip to content

Add sea surface albedo #458

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 7 commits into from
May 18, 2018
Merged

Add sea surface albedo #458

merged 7 commits into from
May 18, 2018

Conversation

tsaoyu
Copy link
Contributor

@tsaoyu tsaoyu commented May 13, 2018

pvlib python pull request guidelines

Thank you for your contribution to pvlib python!

You may submit a pull request with your code at any stage of completion, however, before the code can be merged the following items must be addressed:

  • Closes issue #xxxx
  • 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.

Please don't hesitate to ask for help if you're unsure of how to accomplish any of the above. You may delete all of these instructions except for the list above.

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

This commit added sea surface albedo to SURFACE_ALBEDOS. Technical reference come from here

Also a quick question on the permission of irradiance.py is-rwxr-xr-x done for purpose?

@cwhanse
Copy link
Member

cwhanse commented May 14, 2018

@tsaoyu thanks for this addition. Please update the signature of grounddiffuse to allow surface_type to have a new keyword value 'sea'.

The value 0.04 appears to be a lower bound of some sort on sea surface albedo under clear skies. I looked at the reference you linked, particularly at Table 5. In my view, 0.06 is a more appropriate choice. 0.06 is also what is published at https://en.wikipedia.org/wiki/Albedo

We should also fix the references. The albedo values are referenced in grounddiffuse, where the SURFACE_ALBEDO dict is used. The reference comments should be moved to be adjacent to the dict's declaration. Also, please change the reference to pvpmc.sandia.gov/... to PVSyst v6 at http://files.pvsyst.com/help/albedo.htm, which is where we obtained the values published in the matlab code. Please add the reference you linked as well.

Thanks!

@tsaoyu
Copy link
Contributor Author

tsaoyu commented May 16, 2018

Sure, please review these commits.

Cheers

@@ -489,9 +489,11 @@ def grounddiffuse(surface_tilt, ghi, albedo=.25, surface_type=None):
The calculation is the last term of equations 3, 4, 7, 8, 10, 11, and 12.

[2] albedos from:
Copy link
Member

Choose a reason for hiding this comment

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

Please move these comment lines to where SURFACE_ALBEDO is defined, unless that violates some python grammar rule (@wholmgren ??). You can drop the '[2]' Thanks again for these changes.

Copy link
Member

Choose a reason for hiding this comment

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

@cwhanse I think makes sense to continue to include this reference here (in the References section of the grounddiffuse docstring). This reference will be rendered into a readthedocs page, whereas a code comment will be hidden.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you both. I have added inline comments at line 20.

Copy link
Member

@wholmgren wholmgren left a comment

Choose a reason for hiding this comment

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

Please also pull in the 0.6.0 whatsnew file that was recently added to master, and add a note and your name to it.

@@ -489,9 +489,11 @@ def grounddiffuse(surface_tilt, ghi, albedo=.25, surface_type=None):
The calculation is the last term of equations 3, 4, 7, 8, 10, 11, and 12.

[2] albedos from:
Copy link
Member

Choose a reason for hiding this comment

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

@cwhanse I think makes sense to continue to include this reference here (in the References section of the grounddiffuse docstring). This reference will be rendered into a readthedocs page, whereas a code comment will be hidden.

@@ -30,7 +30,8 @@
'aluminum': 0.85,
'copper': 0.74,
'fresh steel': 0.35,
'dirty steel': 0.08}
'dirty steel': 0.08,
'sea': 0.06}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add # see References section of grounddiffuse function to line 20.

Copy link
Member

Choose a reason for hiding this comment

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

That works too

@wholmgren
Copy link
Member

Thanks @tsaoyu. I think we are ready to merge. I believe the Travis build failure on Python 3.6 is due to a new pandas 0.23 feature concerning preserving dict-order insertion.

@tsaoyu
Copy link
Contributor Author

tsaoyu commented May 18, 2018

Thanks ! @wholmgren

@wholmgren wholmgren added this to the 0.6.0 milestone May 18, 2018
@wholmgren wholmgren merged commit bd13a1b into pvlib:master May 18, 2018
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