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

[pesaventofilippo]

Describe the issue:

This is not really a bug, rather an inconsistency in a very specific case.
In Mexico FP1, POU did 4 laps, but all of them had no data (specifically, LapTime is NaN).

In that case, using laps.pick_fastest(only_by_time=False) works and returns (I think) the first lap, although all data is NaN, while using laps.pick_fastest(only_by_time=True) throws ValueError: attempt to get argmin of an empty sequence (obviously because there's no lap times to compare).

Sometimes it could be useful to check if a lap's time is NaN after picking it with pick_fastest(), so I think we should return the first lap (or any) if all of them are NaN. But let me know what you think.

Reproduce the code example:

import fastf1
session = fastf1.get_session(2023, "Mexico", "FP1")
session.load()

laps = session.laps.pick_driver("POU")
flap = laps.pick_fastest()
print(flap.LapTime) # nan

flap = laps.pick_fastest(only_by_time=True) # ValueError

Error message:

>>> laps.pick_fastest(only_by_time=True)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/site-packages/fastf1/core.py", line 2818, in pick_fastest
    lap = laps.loc[laps['LapTime'].idxmin()]
                   ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/site-packages/pandas/core/series.py", line 2460, in idxmin
    i = self.argmin(axis, skipna, *args, **kwargs)  # type: ignore[arg-type]
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/site-packages/pandas/core/base.py", line 738, in argmin
    return delegate.argmin()
           ^^^^^^^^^^^^^^^^^
  File "/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/site-packages/pandas/core/arrays/_mixins.py", line 201, in argmin
    return nargminmax(self, "argmin", axis=axis)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/site-packages/pandas/core/sorting.py", line 473, in nargminmax
    return _nanargminmax(arr_values, mask, func)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/site-packages/pandas/core/sorting.py", line 484, in _nanargminmax
    return non_nan_idx[func(non_nans)]
                       ^^^^^^^^^^^^^^
  File "/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/site-packages/numpy/core/fromnumeric.py", line 1325, in argmin
    return _wrapfunc(a, 'argmin', axis=axis, out=out, **kwds)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/site-packages/numpy/core/fromnumeric.py", line 59, in _wrapfunc
    return bound(*args, **kwds)
           ^^^^^^^^^^^^^^^^^^^^
ValueError: attempt to get argmin of an empty sequence

[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.