`get_sun_rise_set_transit()` results on wrong day

[mdeceglie]

Describe the bug location.Location.get_sun_rise_set_transit() yields results that are time stamps on the wrong day

To Reproduce

import pvlib
import pandas as pd
loc = pvlib.location.Location(39.74, -105.17, tz='America/Denver')
times = pd.date_range('2023/01/01 00:00', '2023/01/02 12:00', freq='4H', tz='America/Denver')
result = loc.get_sun_rise_set_transit(times)

Expected behavior sunrise, sunset, transit occur on the day of the timestamp in the timezone of times (or maybe loc? I'm not sure what the expected behavior should be when those don't match...)

Screenshots

Versions:

• pvlib.__version__: 0.9.4
• pandas.__version__: 1.4.2
• python: 3.9.12

Additional context Seems related to #316. Thanks to @silverman for finding this bug

[kandersolar]

Location.get_sun_rise_set_transit defaults to method='pyephem', meaning solarposition.sun_rise_set_transit_ephem is the relevant underlying function here. It takes a next_or_previous parameter which defaults to next, so I think the results in that screenshot are consistent with the function's documentation.

What seems inconsistent to me is that this next/previous concept doesn't exist in solarposition.sun_rise_set_transit_spa, which I think returns the values for the specified date regardless of whether they occur before or after the specified time. If we want to keep this difference in behavior, we should probably add a note to the Location method's docstring that the returned date depends on method.

316 seems to be related to the SPA implementation, so not directly relevant here. However for completeness @mdeceglie and @silverman it may be worth confirming that #1449 is not affecting your results.

[cwhanse]

Strictly speaking, not a bug since solarposition.sun_rise_set_transit_ephem (default) returns the next event after each time stamp. But I agree, this isn't the best behavior, and isn't explained or suggested in the Location method's docstring.

[cwhanse]

pyephem is in maintenance-only mode, the developers are pointing to Skyfield as preferred for new work. Could be time to move away from this dependency.

[mdeceglie]

I suggest that this aspect of the results (the convention of whether it is next, on the date, or previous) shouldn't depend on the method passed to get_sun_rise_set_transit(). If we are going to keep pyephem as a method, I'd suggest doing something in the pvlib code to make the results obey the same convention regardless of the method. Perhaps creating an intermediate daily dataframe with just midnight of the relevant dates as the index and then joining that by date to the original index.

[cwhanse]

I'm vaguely remembering dealing with this question when implementing the ephem functions. Defining sunrise on a date is relative to the timezone. The Location method has a location so it can unambiguously define sunrise relative to local midnight. That's not really possible for the solarposition functions, which is what led to the next kwarg.