Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add warnings to users when identifier is fuzzy matched #574

Merged
merged 2 commits into from
Apr 23, 2024

Conversation

Casper-Guo
Copy link
Contributor

@Casper-Guo Casper-Guo commented Apr 19, 2024

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

fastf1/plotting.py Outdated Show resolved Hide resolved
@theOehrly
Copy link
Owner

That's a good idea, actually.

@theOehrly
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

Sounds good to me

@theOehrly theOehrly merged commit c9c7990 into theOehrly:master Apr 23, 2024
10 checks passed
@Casper-Guo Casper-Guo deleted the name-warn branch April 24, 2024 00:53
@Casper-Guo Casper-Guo mentioned this pull request May 13, 2024
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants