search

theOehrly / Fast-F1

FastF1 is a python package for accessing and analyzing Formula 1 results, schedules, timing data and telemetry
https://docs.fastf1.dev
MIT License
2.29k stars 239 forks source link

Add warnings to users when identifier is fuzzy matched #574

Closed Casper-Guo closed 2 months ago

Casper-Guo commented 2 months ago

I think we should issue a warning to users when they provide an invalid input that is fuzzy corrected, just for awareness.

theOehrly commented 2 months ago

That's a good idea, actually.

theOehrly commented 2 months ago

Events.get_event_by_name might benefit from a similar warning in the else where fuzzy matching is done. Or did you have a reason for specifically only adding this to the plotting functions?

Casper-Guo commented 2 months ago

I don't know the appropriate condition for emitting a warning in Events.get_event_by_name.

For example, supplying "Italian" to that function will return the Italian Grand Prix correctly. Based on my reading of the implementation, the EventName entry is modified to Italian. Since fuzz.ratio is sensitive to whitespace, this is not a 100% match. A warning here would just be confusing.

We can maybe use fuzz.partial_ratio instead and emit a warning when there's no 100% match, although I think that might not be bulletproof either.

Or we can get rid of all whitespaces in the strings first before comparing them.

theOehrly commented 2 months ago

Are you interested in figuring that out and changing the fuzzy matching code there to make it work? Else I'll merge this as it is.

Casper-Guo commented 2 months ago

I'm pretty busy the next few days so a thorough investigation with a test suite will have to wait until next weekend at least. I suppose this is not urgent tho and I am happy to take a look at it.

theOehrly commented 2 months ago

Actually, with me working on the complete overhaul of the plotting code (to be done soon, hopefully), I'd be happy if I could rebase onto the master with these changes applied.

So if you are happy with that, I'd prefer to merge these changes. And then you could make a separate PR for the other changes without any time pressure.

Casper-Guo commented 2 months ago

Sounds good to me