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

Enhance add_sorted_driver_legend #621

Merged
merged 7 commits into from
Jul 25, 2024
Merged

Conversation

formulatimer
Copy link
Contributor

It would be useful if the function fastf1.plotting.add_sorted_driver_legend(ax, term) included the same arguments as the Matplotlib function ax.legend(), such as loc, fontsize...

Another improvement would be to ensure a unique list of labels

@theOehrly
Copy link
Owner

I agree with passing args and kwargs through to Matplotlib. We should do that. But I do not agree with ensuring uniqueness. Matplotlib's ax.legend() does not ensure unique labels either, so I don't think we should modify this part of the default behaviour.

@theOehrly
Copy link
Owner

The docstring also needs entries for args and kwargs then

@theOehrly
Copy link
Owner

I thought about this a bit more, and I think I see what you were aiming for in general. You are right that there is not enough customizability here right now.
The correct solution would probably be to explicitly take handles and labels as optional arguments and then only add those instead of getting all labels for the plot. The call signature for axes.legend is annoyingly convoluted and not that easy to overload.

@formulatimer
Copy link
Contributor Author

When you call add_sorted_driver_legend(ax, term), the only correct labels are drivers. Since plotting duplicate drivers makes no sense, I thought that unique label would be useful (at least for me)

I will add args and kwargs to the docstring

@theOehrly I saw you merge the new plotting module but no version has been released yet, how could I use the latest version of the plotting module?

@formulatimer
Copy link
Contributor Author

Maybe this will not be the best solution for the future but for now is a very good solution and I need to customize the legend to adapt my code to the new plotting module. I would like to use the new plotting module in Belgium

@theOehrly
Copy link
Owner

theOehrly commented Jul 24, 2024

Maybe this will not be the best solution for the future but for now is a very good solution

The problem with that is that there should be API consistency. Changing the behaviour and function signature should be done with proper deprecation cycles, and those are slow. Therefore, I'd like to not need to touch this again soon.

I'll take the liberty to push a change to this branch and then merge (today or tomorrow). After the change, the function signature will behave exactly as axes.legend() from Matplotlib does. That means, you can pass customized handles and labels and remove duplicates by yourself before calling this function with a customized version of the handles and labels. This way, it works exactly as it would for vanilla Matplotlib.

My reasons for not doing deduplication in this function are that first, it might be surprising to other users because it doesn't match Matplotlib's behaviour. Second, if we do deduplication, there's no way around it. People cannot opt-out except if we add yet another argument and make the signature even more different from axes.legend(). Now, by default duplicates will be kept. But users can manually remove duplicates beforehand.

@theOehrly
Copy link
Owner

The release will be tomorrow if I managed to verify and create a release with both Ergast and the F1 Livetiming API having problems right now.

@theOehrly theOehrly merged commit 25698cb into theOehrly:master Jul 25, 2024
8 of 9 checks passed
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