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

Fix legend overlapping with lines on causal impact plots #341

Open
1 task
drbenvincent opened this issue May 8, 2024 · 5 comments
Open
1 task

Fix legend overlapping with lines on causal impact plots #341

drbenvincent opened this issue May 8, 2024 · 5 comments
Labels
enhancement New feature or request good first issue Good for newcomers plotting Improve or fix plotting

Comments

@drbenvincent
Copy link
Collaborator

In a number of situations, the legend is overlapping with data in the causal impact plots. Here's an example:
Screenshot 2024-05-08 at 21 06 46

  • Create a kwarg in the plot method which allows users to specify that they want the legend outside the plot. It might take some experimentation to get this right. For example, does it look best to put the legend on the side of the plots (but this may look strange) or perhaps under the bottom?
@drbenvincent drbenvincent added enhancement New feature or request good first issue Good for newcomers plotting Improve or fix plotting labels May 8, 2024
@anevolbap
Copy link
Contributor

anevolbap commented May 14, 2024

Adding something like this should work:
sns.move_legend(ax[0], "upper left", bbox_to_anchor=(1, 1))

Maybe figsize can be adjusted to make wider as before 🤔

image

@drbenvincent
Copy link
Collaborator Author

Sorry for slow reply @anevolbap - notification got buried.

I think that could be good, though I'd prefer direct matplotlib manipulation rather than seaborn, I think it's still just a 1 liner.

I'm also wondering about placing the legend at the bottom. Maybe we can have a kwarg for the plot method where we can ask for "bottom" or "right" for example.

I can't remember now, but I believe we pass back the figure and axis handles, so if a user wanted fine-grained control then they could do that after the call to result.plot().

@anevolbap
Copy link
Contributor

No worries, @drbenvincent. I'm quite busy over here too. I'll work on your comments. Thanks!

@OriolAbril
Copy link
Contributor

Instead of using seaborn's move_legend (which actually deletes the legend and creates a new one under the hood), you can pass these same arguments to the call to ax.legend. Would the goal be to allow users to pass arbitrary kwargs to ax.legend or would it be preferrable to have a single legend kwarg that takes a few strings representing common placements?

@drbenvincent
Copy link
Collaborator Author

I don't have strong feelings about it, but my slight preference would be towards allowing the user more control. As long as we have an example (and info in the plot docstrings) then it shouldn't be too tricky for people to figure out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers plotting Improve or fix plotting
Projects
None yet
Development

No branches or pull requests

3 participants