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: figure_title and chart_title were not mapped up correctly #1676

Merged
merged 4 commits into from
Dec 19, 2023

Conversation

mofojed
Copy link
Member

@mofojed mofojed commented Dec 7, 2023

from deephaven.plot.figure import Figure
from deephaven import read_csv

insurance = read_csv("https://media.githubusercontent.com/media/deephaven/examples/main/Insurance/csv/insurance.csv")

insurance_by_region = insurance.view(formulas=["region", "expenses"]).avg_by(["region"])
insurance_by_sex = insurance.view(formulas=["sex", "expenses"]).avg_by(["sex"])
insurance_by_children = insurance.view(formulas=["children", "expenses"]).avg_by(["children"]).sort(order_by=["children"])
insurance_by_smoker = insurance.view(["smoker", "expenses"]).avg_by(["smoker"])

insurance_cat_plots = Figure(rows=2, cols=2).\
    new_chart(row=0, col=0).\
    plot_cat(series_name="Region", t=insurance_by_region, category="region", y="expenses").\
    chart_title(title="Average charge ($) by region").\
    new_chart(row=0, col=1).\
    plot_cat(series_name="Sex", t=insurance_by_sex, category="sex", y="expenses").\
    chart_title(title="Average charge ($) by sex").\
    new_chart(row=1, col=0).\
    plot_cat(series_name="Children", t=insurance_by_children, category="children", y="expenses").\
    chart_title(title="Average charge ($) per number of children").\
    new_chart(row=1, col=1).\
    plot_cat(series_name="Smoker", t=insurance_by_smoker, category="smoker", y="expenses").\
    chart_title(title="Average charge ($) smokers vs nonsmokers").\
    chart_legend(visible=False).\
    figure_title("Figure Title").\
    show()

image

@mofojed mofojed added the bug Something isn't working label Dec 7, 2023
@mofojed mofojed self-assigned this Dec 7, 2023
Copy link

codecov bot commented Dec 7, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (ba1d51b) 46.65% compared to head (9ee6592) 46.67%.

Files Patch % Lines
packages/chart/src/FigureChartModel.ts 92.30% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1676      +/-   ##
==========================================
+ Coverage   46.65%   46.67%   +0.02%     
==========================================
  Files         606      606              
  Lines       36852    36873      +21     
  Branches     9255     9267      +12     
==========================================
+ Hits        17192    17211      +19     
- Misses      19608    19610       +2     
  Partials       52       52              
Flag Coverage Δ
unit 46.67% <93.54%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mofojed mofojed requested a review from dsmmcken December 7, 2023 19:18
@dsmmcken
Copy link
Contributor

dsmmcken commented Dec 8, 2023

Previously for some reason we were mapping the first subplot chart title to the figure title, which was incorrect. Instead map the Figure title attribute to the figure, and then chart title to the individual subplots

I think we need to still support this behaviour as that's what our docs said to use, so people have been using chart_title like it was figure_title. Maybe only in the case where figure_title is null or chart_title count == 1 or something.

- We currently were showing the first charts title as the figure title
- Needed to add annotations for it
- Fixes deephaven#1675
- Fallback to chart title if there is only one chart and the figure title is not set
- Use `yshift` property to position the text correctly regardless of size of plot
Copy link
Collaborator

@mattrunyon mattrunyon left a comment

Choose a reason for hiding this comment

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

Looks good. Just want to confirm w/ @dsmmcken that the chart title spacing is good (it's consistent now regardless of panel size)

newplot

@dsmmcken
Copy link
Contributor

Spacing is fine :shipit:

@mofojed mofojed merged commit 73e0b65 into deephaven:main Dec 19, 2023
5 checks passed
@mofojed mofojed deleted the 1674-figure-title branch December 19, 2023 15:11
@github-actions github-actions bot locked and limited conversation to collaborators Dec 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

chart_title doesn't title subplots Figure().figure_title() does not render a title in the IDE or in Jupyter
3 participants