Skip to content

fix erbs near zenith=90 #683

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 5 commits into from
Apr 5, 2019
Merged

fix erbs near zenith=90 #683

merged 5 commits into from
Apr 5, 2019

Conversation

wholmgren
Copy link
Member

@wholmgren wholmgren commented Mar 29, 2019

  • Closes irradiance.erbs behavior near zenith = 90 #681
  • 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.

@wholmgren wholmgren added bug solarfx2 DOE SETO Solar Forecasting 2 / Solar Forecast Arbiter labels Mar 29, 2019
@wholmgren wholmgren added this to the 0.6.2 milestone Mar 29, 2019
@wholmgren wholmgren requested a review from cwhanse March 29, 2019 22:10
@wholmgren
Copy link
Member Author

Merging tomorrow unless anyone speaks up...

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.

Just a thought for the future: if we find another place where the bad_data filter is needed, I suggest that a helper function would be useful.

* `erbs` DataFrame vs. OrderedDict return behavior now determined by type
of `datetime_or_doy` instead of `ghi` or `zenith`.
* Added `min_cos_zenith` and `max_zenith` keyword arguments to `erbs`.
(:issue:`681`)
Copy link
Member

Choose a reason for hiding this comment

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

might need double ticks here (and below) for ReStructured Text - this always get's me - single ticks is for markdown

* func:`~pvlib.irradiance.erbs.erbs` ``doy`` argument changed to ``datetime_or_doy`` to be consistent with
  allowed types and similar functions (``disc``, ``get_extra_radiation``).
* func:`~pvlib.irradiance.erbs.erbs` DataFrame vs. OrderedDict return behavior now determined by type
  of ``datetime_or_doy`` instead of ``ghi`` or ``zenith``.
* Added ``min_cos_zenith`` and ``max_zenith`` keyword arguments to func:`~pvlib.irradiance.erbs.erbs`.
  (:issue:`681`)

Copy link
Member Author

Choose a reason for hiding this comment

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

The Python docs seem to consistently use italics for parameters and links for functions. I don't think we have a standard here (though I recall you've done some work on this in older whats new files).

Copy link
Member

Choose a reason for hiding this comment

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

I think in rst single ticks get interpreted as italics just like * do, anyway, not a blocker for me, so I defer to you, everything else looks great! Thanks for checking closure on ghi = dni*cos(ze)+dhi 😸

@@ -28,6 +33,7 @@ Bug fixes
* pvwatts_ac raised ZeroDivisionError when called with scalar pdc=0
and a RuntimeWarning for array(0) input. Now correctly returns 0s
of the appropriate type. (:issue:`675`)
* Fixed `erbs` behavior when zenith is near 90 degrees. (:issue:`681`)
Copy link
Member

Choose a reason for hiding this comment

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

double ticks for rst or use func directive to erbs in api

kt = ghi / i0_h
kt = np.maximum(kt, 0)
kt = clearness_index(ghi, zenith, dni_extra, min_cos_zenith=min_cos_zenith,
max_clearness_index=1)
Copy link
Member

Choose a reason for hiding this comment

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

nice 🚀

bad_values = (zenith > max_zenith) | (ghi < 0) | (dni < 0)
dni = np.where(bad_values, 0, dni)
# ensure that closure relationship remains valid
dhi = np.where(bad_values, ghi, dhi)
Copy link
Member

Choose a reason for hiding this comment

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

very nice! 🚀

Copy link
Member

@mikofski mikofski left a comment

Choose a reason for hiding this comment

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

  • looks great, very detailed testing, thanks! 🚀
  • check the rst markup in what's new
  • agree with @cwhanse if bad behavior checks become commonplace, then a helper and perhaps some top-level constants for min_cos_zenith and max_zenith defaults could be nice too

@wholmgren
Copy link
Member Author

rendered whats new looks like this on my machine Screen Shot 2019-04-04 at 12 54 40 PM

@wholmgren wholmgren merged commit 0e9d25c into pvlib:master Apr 5, 2019
@wholmgren wholmgren deleted the erbslim branch April 5, 2019 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug solarfx2 DOE SETO Solar Forecasting 2 / Solar Forecast Arbiter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants