Perform dayofyear-based calculations according to UTC, not local time

[yhkee0404]

• Closes Clearsky functions handling tz-aware datetimes as tz-naive, contrary to user guide #2054
• I am familiar with the contributing guidelines
• Tests added
• 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.

A similar solution as #333

[cwhanse]

@yhkee0404 did you close this intentionally? If you aren't able to complete the PR, let us know and one of us could take it from here.

[yhkee0404]

@cwhanse Oh, sorry for confusion. I was in the midst of my struggle with the problem and was going to reopen it after several hours. Thank you for your suggestions! Now I think it is better to keep it open. Any help would be welcome!

[cwhanse]

Unless objections are stated, I intend to merge this on 9/6

[kandersolar]

I am taking a look at this PR now. Apologies for the delay @yhkee0404!

[kandersolar]

Thanks @yhkee0404 for this thorough PR! I have a few questions below that I hope you can help me understand.

I also think the title of this PR should be edited to reflect the broader scope of the changes.

[kandersolar]

I'm not sure I agree with this change. The docstring says the input timestamps "must be localized to the timezone for the longitude", which is inconsistent with the previous behavior (localize to UTC if not already localized), so I can see some change being needed here. But why is this the correct change?

[yhkee0404]

It was added in accordance with sun_rise_set_transit_spa and sun_rise_set_transit_ephem as well as the docstring. I agree with the previous convention of raising a ValueError since it prevents users from unwittingly calling the functions naively without assuming any timezones and ignoring the fact that an hour angle is counted up from zero at noon in a predefined local timezone. Please refer to the followings:

pvlib-python/pvlib/solarposition.py

Lines 448 to 452 in 524fa55

	# times must be localized
	if times.tz:
	tzinfo = times.tz
	else:
	raise ValueError('times must be localized')

pvlib-python/pvlib/solarposition.py

Lines 560 to 564 in 524fa55

	# times must be localized
	if times.tz:
	tzinfo = times.tz
	else:
	raise ValueError('times must be localized')

[yhkee0404]

Many thanks to @cwhanse for leading this PR into the right way, and @kandersolar for taking time for a further reconciliation from other aspects! Please feel free to fix or revert any including the title or whatsnew with your excellent ideas.

[cwhanse]

@yhkee0404 we should add a few tests to improve coverage. Let me know if you would like my help with that.

[yhkee0404]

@cwhanse I have just made some patchs to improve coverage but I am not sure they would work. I would really appreciate if you could please help me. This is my first time to work with Codecov. Is there a way to see the difference locally on my own, or should I just wait for your manual approval for codecov workflow?

[cwhanse]

@yhkee0404 the additional tests are good enough. I don't see why codecov is flagging that one line. Like you I don't know how to run codecov locally, and just let the CI do it for me.

[yhkee0404]

Great! It seems calling solarposition.spa_c in test_spa_c_physical and test_spa_c_physical_dst on pvlib/tests/test_solarposition.py both fail on Codecov because of lack of spa_c_files which makes these lines fail accordingly: from pvlib.spa_c_files.spa_py import spa_calc, has_spa_c and requires_spa_c. It is different from the case of other libraries like ephem and numba since both has_ephem and has_numba are True on Codecov while has_spa_c() is evaluated as False: https://app.codecov.io/gh/pvlib/pvlib-python/pull/1944/blob/pvlib/tests/conftest.py#L136 In another word, to test spa_c which is not imported, we should install it on Codecov somehow like this direction: https://pvlib-python.readthedocs.io/en/stable/user_guide/installation.html?highlight=spa_c_files#nrel-spa-algorithm

[kandersolar]

I believe I have finally understood this PR. It it looks correct to me. I have only a couple revisions to the what's new text to fully communicate the changes.

In another word, to test spa_c which is not imported, we should install it on Codecov somehow like this direction

Unfortunately, licensing reasons prevent us from testing the spa_c code in CI. Any coverage issues related to spa_c should be ignored :)

[yhkee0404]

Thank you for your thorough review and suggestion that clarifies this PR in more datail👍!

[kandersolar]

Thanks again @yhkee0404 for finding, investigating, and fixing this complex issue!