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

ENH: infinite sheds #717

Merged
merged 133 commits into from
Feb 22, 2022
Merged

ENH: infinite sheds #717

merged 133 commits into from
Feb 22, 2022

Conversation

mikofski
Copy link
Member

@mikofski mikofski commented May 10, 2019

  • [ ] Closes issue #xxxx
  • 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 LGTM and SticklerCI checks.
  • 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.

Implements "infinite sheds" model for irradiance on long, regular bifacial array (fixed or single-axis tracked) on horizontal ground.

* add _to_radians and is_rad to convert only if necessary
* prefix all functions with get_
* add get_f_sky_pv, get_poa_sky_pv, get_ground_angle_tangent,
get_f_gnd_pv, get_f_gnd_pv, get_poa_gnd_pv, etc.
* update API ui

Signed-off-by: Mark Mikofski <[email protected]>
* get_irradiance, output ordered dict or dataframe
* finish updating get_poa_global_bifacial to transpose beam and diffuse
for each side separately
…ion"

- use tan(zenith) in solar projection math latex
- implement gcr_prime and ground-sky-angles calculations
- add stub for ground-diffuse view factor

Signed-off-by: Mark Mikofski <[email protected]>
- calculate ground-sky angles to previous and next rows, assuming height
 is nonzero
- calculate limits on ground where it can see the sky
- calculate the view factor as a function of z on the ground to the sky
- fix places where it still says degrees, bad, no!
- add fixme for pv-sky view factor, still has wrong formula
- add tests for angles from point z on the ground to tops of current
 row, and limits of previous and next rows
- add a script to make the plot of ground-sky view factor versus z
@mikofski
Copy link
Member Author

mikofski commented Jul 4, 2019

Sorry for the delay. Getting closer! Here's the view factor of the sky diffuse from the ground between rows as a function of point z on the ground between rows, starting where the line from the panel intersects the ground:
pvlib_infinite_sheds_fgroundz-sky
this is generated by a script in pvlib/tests/test_infinite_sheds.py

@mikofski
Copy link
Member Author

mikofski commented Jul 4, 2019

@wholmgren FYI: looks like pandas and other versions not supported by python-3.5, not sure?

- add comments, change names x->z
- add TODO's to limit number of rows, and set row-type: 'first', 'last',
or 'middle'
- was difference of angles, should be difference of cosines
- also add TODO's to return VF versus point x on panel, and don't use
averages
@wholmgren
Copy link
Member

wholmgren commented Jul 4, 2019 via email

- change calc_fx_sky to calc_fz_sky since z is for ground and x is for
pv surface
- add docstring to calc_fz_sky and for ground_sky_diffuse_view_factor
@cwhanse
Copy link
Member

cwhanse commented Feb 13, 2022

Terminology issue: pvlib isn't consistent in terms referring to solar zenith and solar azimuth.

The last set of changes here renamed two parameters: solar_zenith to apparent_zenith, and solar_azimuth to azimuth. solar_zenith and solar_azimuth are used in pvlib/tracking.py without specifying specify true or refraction-corrected zenith. In pvlib/irradiance.py, solar_zenith is used for refraction-corrected zenith. apparent_zenith is used in pvlib/solar_position.py for the refraction-corrected value, zenith for the true value, with azimuth.

So pvlib isn't consistent in these terms, and in places is not specific. With that in mind I'm going to open a discussion about the zenith term, so we can have a goal, and this PR can step towards that goal for the bifacial code.

@adriesse
Copy link
Member

In my own code have standardized on zenith to refer to refraction-corrected solar zenith angle. I don't usually need the other one.

@mikofski
Copy link
Member Author

I think this is ready to merge. @wholmgren wanna do the honors?

@mikofski
Copy link
Member Author

If no one has any objections I'll merge this today, COB, okay?

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.

@mikofski thanks, I forgot about this, but I'll let you do the honors when ready. My comments are not blockers.

@@ -0,0 +1,831 @@
r"""
Copy link
Member

Choose a reason for hiding this comment

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

This is a nice module docstring, but I don't think it's rendered anywhere. Did I miss it? I don't recall rendering any module docstrings (for better or worse). I do recall that it might not be super simple for our current documentation strategy. If so, perhaps the doc string could be copied into another place, like a new bifacial.rst or an example gallery py file?

Copy link
Member Author

Choose a reason for hiding this comment

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

@cwhanse any comment on this? Seems like a good idea to me. I happy to move it if you agree. thx.

Copy link
Member

Choose a reason for hiding this comment

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

+1 to bifacial.rst

Comment on lines 466 to 467
def get_irradiance_poa(surface_tilt, surface_azimuth, apparent_zenith,
azimuth, gcr, height, pitch, ghi, dhi, dni,
Copy link
Member

Choose a reason for hiding this comment

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

Given where #1403 is going, I suggest we go with solar_zenith and solar_azimuth like we do here

def get_total_irradiance(surface_tilt, surface_azimuth,
solar_zenith, solar_azimuth,

Copy link
Member Author

Choose a reason for hiding this comment

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

Should we wait until after #1403 is closed to merge this PR?


Returns
-------
output : OrderedDict or DataFrame
Copy link
Member

Choose a reason for hiding this comment

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

A few years later, perhaps we just go with dict instead of OrderedDict?

Copy link
Member

Choose a reason for hiding this comment

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

@wholmgren do you mean change only the docstring, or replace the use of OrderedDict with {}?

Copy link
Member

Choose a reason for hiding this comment

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

Replace the use

pvlib/bifacial/infinite_sheds.py Outdated Show resolved Hide resolved
pvlib/bifacial/infinite_sheds.py Outdated Show resolved Hide resolved
pvlib/bifacial/utils.py Outdated Show resolved Hide resolved
@cwhanse
Copy link
Member

cwhanse commented Feb 17, 2022

If no one has any objections I'll merge this today, COB, okay?

I'd prefer to resolve the solar_zenith question before merging. See discussion in #1403

Copy link
Member

@kandersolar kandersolar left a comment

Choose a reason for hiding this comment

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

A few sphinx rendering issues

docs/sphinx/source/user_guide/bifacial.rst Outdated Show resolved Hide resolved
docs/sphinx/source/user_guide/bifacial.rst Outdated Show resolved Hide resolved
docs/sphinx/source/user_guide/bifacial.rst Outdated Show resolved Hide resolved
docs/sphinx/source/user_guide/bifacial.rst Outdated Show resolved Hide resolved
docs/sphinx/source/user_guide/bifacial.rst Outdated Show resolved Hide resolved
@cwhanse
Copy link
Member

cwhanse commented Feb 21, 2022

@wholmgren @kanderso-nrel I think I have addressed all comments. I'm satisfied that this PR is ready to merge. @mikofski want the honor of pushing the big green button?

surface_azimuth)
f_gnd_beam = 1.0 - np.minimum(
1.0, gcr * np.abs(cosd(surface_tilt) + sind(surface_tilt) * tan_phi))
np.where(solar_zenith > max_zenith, 0., f_gnd_beam) # [1], Eq. 4
Copy link
Contributor

Choose a reason for hiding this comment

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

@mikofski I think this line has no effect as there is no return value.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a point, well spotted @kdebrab ! Would you like to open a PR fixing it?

Copy link
Member Author

Choose a reason for hiding this comment

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

This line is probably supposed to start with f_gnd_beam = and clip it at max zenith so values don’t blow up.
ICYMI @cwhanse @kandersolar

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @mikofski, that was the intent.

Copy link
Member Author

Choose a reason for hiding this comment

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

these lines are still unchanged in main:

    f_gnd_beam = 1.0 - np.minimum(
        1.0, gcr * np.abs(cosd(surface_tilt) + sind(surface_tilt) * tan_phi))
    np.where(solar_zenith > max_zenith, 0., f_gnd_beam)  # [1], Eq. 4
    return f_gnd_beam  # 1 - min(1, abs()) < 1 always

was a PR forthcoming?

Copy link
Member

Choose a reason for hiding this comment

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

@mikofski has not been fixed, should have its own issue.

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.

9 participants