Issue #2245 Infinite sheds beam fraction on ground zenith guardrail syntax bug

[jason-rpkt]

• Closes Infinite sheds beam fraction on ground zenith guardrail syntax bug #2245
• I am familiar with the contributing guidelines
• Tests added
• no changes to tests>bifacial>test_utils.py.
• pytest pvlib/tests/bifacial/test_utils.py collects and passes 18 items
• Updates entries in docs/sphinx/source/reference for API changes.
• 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.

[cwhanse]

My guess is that the test failure is because f_gnd_beam is being returned as a numpy array, after use of np.where, rather than being preserved as a pd.Series. Somehow that is changing the type of poa_global to a numpy array, which interrupts returning the results as a Dataframe.

I'm not sure how to fix this yet.

[echedey-ls]

Thanks @jason-rpkt for the PR!!

@cwhanse is right in his reasoning. I don't think we need to guarantee returning pd.Series inside private functions, so by changing infinite_sheds.py line 375 to compare the type of solar_zenith it should be enough. Please indicate in the docstring that this input is the one that sets the output type (for example, in the returns section).

Also, please add the bugfix and list yourself in docs\sphinx\source\whatsnew\v0.11.3.rst. And finally, have a look at the remaining workflows that have run, you have to fix a PEP8 style issue (not using spaces around = operator).

TIA!!

[cwhanse]

compare the type of solar_zenith

+1, thanks @echedey-ls

[jason-rpkt]

Thank you both!
I have changed the flake8 errors for missing space around = operator and line length.
Commit 8b67a41 on init.py was a mistake. (Very new to this and trying to figure it out)
I'll revert it/ remove these changes.

Sorry, I'm a little confused by this part:
changing infinite_sheds.py line 375 to compare the type of solar_zenith

Do you mean change infinite_sheds.py line 375 from
if isinstance(poa_global, pd.Series): to if isinstance(solar_zenith, pd.Series): ?

Would changing line 375 from
if isinstance(poa_global, pd.Series): to if isinstance(poa_global, np.ndarray): work?

[cwhanse]

if isinstance(poa_global, pd.Series): to if isinstance(solar_zenith, pd.Series): ?

Yes.

Would changing line 375 from
if isinstance(poa_global, pd.Series): to if isinstance(poa_global, np.ndarray): work?

No. pvlib tries to respect the type of inputs and return the same types. If input to a function is pd.Series, that means returning either pd.Series or (in this case) pd.DataFrame. Conversely, the function should return np.ndarray if the inputs are of that type.

Changing as you propose would return pd.DataFrame when the poa_global input is np.ndarray.

[echedey-ls]

It looks great, but I think I should have proposed using ghi instead of solar_zenith in the isinstance function. It was previously that in the documentation.

To continue, you have to update the branch from main, so the documentation can be built without problems and we can check how it renders.
Finally, add an entry to docs\sphinx\source\whatsnew\v0.11.3.rst and credit yourself in the Contributors section!

[echedey-ls]

Now that I read this, I think I should have suggested following the type of ghi instead of solar_zenith. Sorry for the confusion @jason-rpkt .

[echedey-ls]

Suggested change

	Output is a DataFrame when solar_zenith is a Series.
	solar_zenith is the one that sets the output type.
	See Issue #2245
	See Notes for
	descriptions of content.
	Output is a ``pandas.DataFrame`` when ``ghi`` is a Series,
	otherwise it is a dict of ``numpy.ndarray``.
	See Notes for descriptions of content.

[jason-rpkt]

I have updated the fork from main, changed from "solar_zenith" to "ghi" as suggested, and updated docs\sphinx\source\whatsnew\v0.11.3.rst
Please let me know if this works.

[cwhanse]

Tests fail because ghi is a float for that sequence of tests, not a Series.

Please add this line in the group right after line 132

ghi = pd.Series(data=ghi, index=surface_tilt.index)

[cwhanse]

Suggested change

	Output is a "pandas.DataFrame" when "ghi" is a Series.
	Otherwise it is a dict of "numpy.ndarray"
	See Issue #2245
	See Notes for descriptions of content.
	Output is a ``pandas.DataFrame`` when ``ghi`` is a Series.
	Otherwise it is a dict of ``numpy.ndarray``.
	See Notes for descriptions of content.

[cwhanse]

Sorry about that double recommendation, it's my company firewall getting in the way of following links.

[jason-rpkt]

ghi = pd.Series(data=ghi, index=surface_tilt.index) should this be after line 132? That would be in _shaded_fraction which does not have ghi as input.
Should I add it as first line of get_irradiance_poa, line 294 instead?

[echedey-ls]

ghi = pd.Series(data=ghi, index=surface_tilt.index) should this be after line 132? That would be in _shaded_fraction which does not have ghi as input. Should I add it as first line of get_irradiance_poa, line 294 instead?

@cwhanse was referring to file test_infinite_sheds.py. test_get_irradiance_poa ensures IO datatypes consistency. Now the output type changes to dataframe if ghi is a series, but as of now, it is always provided as a numpy array.

[echedey-ls]

Suggested change

	Fix syntax for unshaded ground fraction (:issue:`2245`, :pull:`2359`)
	Fix bug in :py:func:`pvlib.bifacial.get_irradiance_poa` which may had yielded non-zero ground irradiance with sun below horizon (:issue:`2245`, :pull:`2359`)

[echedey-ls]

Suggested change

See Issue #2245

[echedey-ls]

Suggested change

	f_gnd_beam = np.where(solar_zenith > max_zenith, 0., f_gnd_beam)
	# [1], Eq. 4
	# [1], Eq. 4
	f_gnd_beam = np.where(solar_zenith > max_zenith, 0., f_gnd_beam)

[cwhanse]

@jason-rpkt I had to fix the merge conflict in whatsnew to start the CI jobs. You will need to git pull origin main (if origin is what you called your fork) to bring the change to your local files, if you need to do any further work.

[echedey-ls]

LGTM!