[BUG] Laps.pick_fastest(only_by_time=True) inconsistency

[pesaventofilippo]

[BUG] Laps.pick_fastest(only_by_time=True) inconsistency

[Casper-Guo]

This seems to be more of a Pandas issue.

Pandas doc for idxmin:

skipna
bool, default True
Exclude NA/null values. If the entire Series is NA, the result will be NA.

This works for and prints out nan:

import numpy as np
test_series = pd.Series([np.nan, np.nan, np.nan])
print(test_series.idxmin())

But this will throw an error:

test_series = pd.Series([pd.NaT, pd.NaT, pd.NaT])
print(test_series.idxmin()

Looks like no one has wanted to fix this issue for a while now.

[Casper-Guo]

Update

Pandas will no longer accommodate series with all null values. So the implementation should be changed.

I think if there is no recorded lap time for any lap then pick_fastest should return None or similar. The return value we get now with only_by_time=False is not populated anyways. It is constructed with Lap(index=self.columns).

[pesaventofilippo]

Good catch. I agree, maybe we should return None if there are no laps, in both cases.
Let's hear what @theOehrly thinks

[theOehrly]

After thinking about this for a bit, I think that returning None is in fact the cleanest solution.

(Intentionally) raising an error is not a good solution because the data is only indirectly provided by the user. Therefore, you can't argue that wrong/incompatible data was given by the user. At the same time, no valid fastest lap is an expected scenario. Thus, this is not really an error.

The current solution of returning an empty lap aims for type consistency but at the same time, dtypes of the values in the lap object are incorrect. So it's not really consistent behaviour either.

But changing this to return None requires a proper deprecation of the current behaviour. I can easily see scripts that would break when this is changed. Therefore, the way to go is to fix the unhandled exception so that an empty Lap object is returned as well. At the same time, add a Deprecation warning that notifies of the upcoming change to returning None instead. The actual change would need to be targeted at v3.3

[theOehrly]

I'll close this one given that this has been addressed, and the pending deprecation has a separate issue as a reminder.