Handle time normalization for nonexistent and ambiguous times

[scttnlsn]

• Closes NonExistentTimeError when calling hour_angle #2132
• I am familiar with the contributing guidelines
• Tests added
• 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`).
• 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]

Fix the whitespace and LGTM

[scttnlsn]

Started running the code from this branch on a real problem and realized the handling of ambiguous='infer' is not sufficient. The issue can be reproduced in the following test case:

eot = np.array([-3.935172, -4.117227, -4.026295])
longitude = 82.3666
times = pd.DatetimeIndex([
    '2014-11-02 09:00:00',
    '2014-11-02 10:00:00',
    '2014-11-02 11:00:00',
]).tz_localize('America/Havana')
solarposition.hour_angle(times, longitude, eot)

We get:

pytz.exceptions.AmbiguousTimeError: There are 2 dst switches when there should only be 1.

I think the infer piece requires exactly 2 duplicate times but we may have more in our times array. Not sure what the best solution is at this point. Any ideas?

[scttnlsn]

I suppose we could look at the UTC offsets to build a list of bools indicating which times are DST. We'd then pass ambigious=dst_list (https://pandas.pydata.org/docs/reference/api/pandas.Series.tz_localize.html).

EDIT: looks like the tzinfo already has info about DST

[cwhanse]

Hmm...I see that I didn't think enough about the tests :) According to the pandas docs infer requires the timestamps to be monotonic, which they aren't after normalize.

[cwhanse]

This passes tests, but is likely slow due to the looping. I'm now thinking that perhaps ambiguous=NaT would be better.

The issue arises only when a user wants to use DST-aware timestamps in a few places and is not careful when constructing the series of timestamps. It seems better to suggest the user localize using a non-DST timezone such as GMT-4. As far as importing data recorded with DST timestamps, if the timestamps are correct, the ambiguous times won't be in the data.

[scttnlsn]

It seems better to suggest the user localize using a non-DST timezone such as GMT-4

That's a good point. In my own use case I could handle the DST stuff before passing the times into hour_angle.

I'm now thinking that perhaps ambiguous=NaT would be better.

I think that would result in nans being returned from hour_angle which I think could be confusing. I think even letting the AmbiguousTimeError be raised is fine if the documentation indicates that DST should be handled outside pvlib. That way the user is forced to address what's happening instead of maybe being confused about receiving nans.

[cwhanse]

I think even letting the AmbiguousTimeError be raised is fine if the documentation indicates that DST should be handled outside pvlib. That way the user is forced to address what's happening instead of maybe being confused about receiving nans.

+1

[scttnlsn]

Do you think I should just close this PR then? For my own use case, if I'm handling the DST stuff outside pvlib anyway then I don't expect to see a NonExistentTimeError either. If you'd like to keep the nonexistent='shift_forward' I can just remove the ambiguous=is_dst stuff.

[cwhanse]

I think we still want to handle the NonExistentTimeError case, but lets raise the AmbiguousTimeError.

[cwhanse]

@scttnlsn can you add yourself to the Contributors list in the whatsnew file? I can do that for you, if needed.

[kandersolar]

Thanks @scttnlsn for the PR! I cannot stand these bizarre timezone issues...

I think even letting the AmbiguousTimeError be raised is fine if the documentation indicates that DST should be handled outside pvlib

Do we still want to add something to the docstring along these lines? I don't see any changes there currently.

[cwhanse]

@pvlib/pvanalytics-maintainer if no objections are raised, I will merge this on 9/13

[yhkee0404]

What about _local_times_from_hours_since_midnight and _times_to_hours_after_local_midnight? I think they too are subject to the same bug of tz_localize and normalize and can be fixed together in the same way easily.

pvlib-python/pvlib/solarposition.py

Lines 1410 to 1419 in cbfa292

	def _local_times_from_hours_since_midnight(times, hours):
	"""
	converts hours since midnight from an array of floats to localized times
	"""
	tz_info = times.tz # pytz timezone info
	naive_times = times.tz_localize(None) # naive but still localized
	# normalize local, naive times to previous midnight and add the hours until
	# sunrise, sunset, and transit
	return pd.DatetimeIndex(
	naive_times.normalize() + pd.to_timedelta(hours, unit='h'), tz=tz_info)

pvlib-python/pvlib/solarposition.py

Lines 1422 to 1426 in cbfa292

	def _times_to_hours_after_local_midnight(times):
	"""convert local pandas datetime indices to array of hours as floats"""
	times = times.tz_localize(None)
	hrs = (times - times.normalize()) / pd.Timedelta('1h')
	return np.array(hrs)

[yhkee0404]

What about this with the modification of def hour_angle(times, longitude, equation_of_time) into def hour_angle(times, longitude, equation_of_time, nonexistent='shift_forward', ambiguous='raise') to give users a choice to control the tz_localize behavior if they want rather than just receiving an error and being blocked?

Suggested change

	# Use Pandas functionality for shifting nonexistent times forward
	normalized_times = naive_normalized_times.tz_localize(
	times.tz, nonexistent='shift_forward', ambiguous='raise')
	# Use Pandas functionality for shifting nonexistent times forward
	normalized_times = naive_normalized_times.tz_localize(
	times.tz, nonexistent=nonexistent, ambiguous=ambiguous)

[AdamRJensen]

@scttnlsn could you comment on this?

[scttnlsn]

The user would not necessarily be blocked if the exception is raised. They'd just need to convert the given times to a non-DST timezone ahead of calling hour_angle. The documentation is updated to mention this.

infer won't really work as mentioned here: #2133 (comment) The other options are to raise (current approach) or to have NaNs returned where the times are ambiguous (the user still likely needs to deal with that case).

I'm personally of the opinion that forcing the user to convert to a non-DST timezone is a fine approach but I could understand someone might want to have NaNs instead.