Skip to content
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

Update get_ground_diffuse description #1883

Merged
merged 6 commits into from Feb 1, 2024

Conversation

AdamRJensen
Copy link
Member

  • I am familiar with the contributing guidelines
  • Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/whatsnew for all changes. Includes link to the GitHub Issue with :issue:`num` or this Pull Request with :pull:`num`. Includes contributor name and/or GitHub username (link with :ghuser:`user`).
  • New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
  • Pull request is nearly complete and ready for detailed review.
  • Maintainer: Appropriate GitHub Labels (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

There exist several methods in the literature for estimating ground-reflected diffuse irradiance. Thus, the pvlib method should clearly state that it is based on isotropic ground reflections (which is the most common method)

@AdamRJensen AdamRJensen added this to the v0.10.3 milestone Oct 5, 2023
Copy link
Member

@cwhanse cwhanse left a comment

Choose a reason for hiding this comment

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

I approve of the clarification.

Fussing about words: should it be "The ground is assumed to be isotropic and reflections are Lambertian".

@@ -542,7 +542,8 @@ def poa_components(aoi, dni, poa_sky_diffuse, poa_ground_diffuse):
def get_ground_diffuse(surface_tilt, ghi, albedo=.25, surface_type=None):
'''
Estimate diffuse irradiance from ground reflections given
irradiance, albedo, and surface tilt.
irradiance, albedo, and surface tilt. The ground is assumed to
be isotropic and reflections are Lambertian.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
be isotropic and reflections are Lambertian.
be horizontal and flat, and reflections are Lambertian.

I don't think the word isotropic is appropriate here.

Copy link
Member

Choose a reason for hiding this comment

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

why not?

Copy link
Member

Choose a reason for hiding this comment

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

Adding "horizontal and flat" is a good idea. isotropic refers to properties such as albedo

Copy link
Member

Choose a reason for hiding this comment

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

If you want to use the word isotropic I would suggest something more like: the surface is Lambertian and the reflected radiance is isotropic.

Copy link
Member

Choose a reason for hiding this comment

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

I can live with that.

pvlib/irradiance.py Outdated Show resolved Hide resolved
@jules-ch
Copy link
Contributor

+1 on this clarification

@cwhanse cwhanse mentioned this pull request Dec 8, 2023
15 tasks
@kandersolar kandersolar modified the milestones: v0.10.3, v0.10.4 Dec 20, 2023
@cwhanse
Copy link
Member

cwhanse commented Feb 1, 2024

@kandersolar @AdamRJensen please take a look, moved this forward to v0.10.4

@AdamRJensen
Copy link
Member Author

@cwhanse Do you have a source that explains the difference between isotropic and Lambertian?

@cwhanse
Copy link
Member

cwhanse commented Feb 1, 2024

@cwhanse Do you have a source that explains the difference between isotropic and Lambertian?

If a surface is Lambertian, then reflected radiance is isotropic. https://en.wikipedia.org/wiki/Lambertian_reflectance

@cwhanse
Copy link
Member

cwhanse commented Feb 1, 2024

A surface (or a material) can also be isotropic, which means that its properties don't depend on position or direction.

@AdamRJensen
Copy link
Member Author

This PR looks good to me!

@kandersolar kandersolar merged commit 859a7ce into pvlib:main Feb 1, 2024
34 checks passed
@mikofski
Copy link
Member

mikofski commented Feb 2, 2024

Maybe irrelevant but the opposite of Lambertian is “specular” like a mirror, a specular surface only reflects light in a single direction: https://en.m.wikipedia.org/wiki/Specular_reflection. A Lambertian surface reflects light in more than one direction

@AdamRJensen AdamRJensen deleted the ground_diffuse_doc branch March 12, 2024 13: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.

None yet

6 participants