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

Improve plot clarity with combined mean and HDI legend elements #145

Merged
merged 5 commits into from
Jan 4, 2023

Conversation

drbenvincent
Copy link
Collaborator

Closes #142 and #143.

  • Previously the figure legends were quite messy. The main thing this PR does is to create single legend entries which include both the central tendency and the HDI. This is actually a bit involved and so we now build up a list of handles and labels in the plotting functions. With this we can call ax.legend once at the end of the plot functions. See my StackOverflow question and post on Twitter.
  • I updated the plot methods for all the pymc experiments to use this new approach. There is certainly still scope for slight improvement, but I think this is a good step forward in improving the plots.

Examples for synthetic control plots...

Before

Screenshot 2022-12-27 at 20 50 32

After

Note the combined line+shaded region in the legend
Screenshot 2022-12-27 at 20 45 28

Examples for difference in differences

Before

notebooks_did_pymc_banks_17_0-1

After

notebooks_did_pymc_banks_17_0

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@drbenvincent
Copy link
Collaborator Author

  • I should update the plots in README

@drbenvincent
Copy link
Collaborator Author

I originally tried to do this on a better way, with everything handled internally in the plot function. But I ran into problems. It may be that I discovered a bug in matplotlib. See the solution here https://stackoverflow.com/a/74935897 If this can be made to work, it would be much cleaner.

include_label: bool = True,
) -> None:
):
Copy link
Collaborator

@juanitorduz juanitorduz Dec 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type hints are missing

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated in an upcoming commit

Copy link
Collaborator

@juanitorduz juanitorduz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some small suggestions. Mostly regarded commented code. This is very hard to maintain and we always have the git history to come back to them anyway. In addtion, what if we put the legends outside the figure as described in https://stackoverflow.com/questions/4700614/how-to-put-the-legend-outside-the-plot ?

@@ -13,28 +14,46 @@ def plot_xY(
ax: plt.Axes,
plot_hdi_kwargs: Optional[Dict[str, Any]] = None,
hdi_prob: float = 0.94,
label: Optional[str] = "",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember Optional[str] is equivalent to Union[str, None. I think we probably do not want this so we should remove Optional. See #117

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to Union[str, None]. This also allowed me to remove a kwarg

x,
Y,
hdi_prob=hdi_prob,
fill_kwargs={
"alpha": 0.25,
"label": f"{hdi_prob*100}% HDI" if include_label else None,
"label": " ", # f"{hdi_prob*100}% HDI" if include_label else None,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall we start removing commented code? this makes the code hard to maintain.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in an upcoming commit

Comment on lines 52 to 58
# if include_label:
# handles, labels = ax.get_legend_handles_labels()
# ax.legend(
# handles=[(h1, h2) for h1, h2 in zip(handles[::2], handles[1::2])],
# # labels=[l1 + " + " + l2 for l1, l2 in zip(labels[::2], labels[1::2])],
# labels=[l1 for l1 in labels[::2]],
# )
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here regarding commented code

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in an upcoming commit

alpha=0.25,
label="Causal impact",
# label="Causal impact",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this duplicated line

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in an upcoming commit

@@ -189,10 +209,14 @@ def plot(self):
ls="-",
lw=3,
color="r",
label="Treatment time",
# label="Treatment time",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove commented line?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in an upcoming commit

@drbenvincent
Copy link
Collaborator Author

Thanks for the comments @juanitorduz. I believe I've dealt with all those changes except for an external axis legend. I agree that it's suboptimal for the synthetic control plots, but I'll improve it in a separate PR.

I also re-ran some notebooks because I was not fond of the green shaded region for the causal impact.

@@ -137,7 +136,6 @@ def plot(self):
self.datapost.index,
self.post_pred["posterior_predictive"].mu,
ax=ax[0],
include_label=False,
# label="Synthetic control",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall we remove this line?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do

@juanitorduz
Copy link
Collaborator

Just left a comment regarding a commented line: either remove or uncomment ;) Feel free to merge.

@drbenvincent drbenvincent merged commit 2b1d82a into main Jan 4, 2023
@drbenvincent drbenvincent deleted the improve-plot-legends branch January 4, 2023 14:06
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.

Improve plots with combined line + credible region in legend
2 participants