Skip to content

fix: figure_title and chart_title were not mapped up correctly#1676

Merged
mofojed merged 4 commits intodeephaven:mainfrom
mofojed:1674-figure-title
Dec 19, 2023
Merged

fix: figure_title and chart_title were not mapped up correctly#1676
mofojed merged 4 commits intodeephaven:mainfrom
mofojed:1674-figure-title

Conversation

@mofojed
Copy link
Copy Markdown
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 requested a review from mattrunyon December 7, 2023 17:28
@mofojed mofojed self-assigned this Dec 7, 2023
@codecov
Copy link
Copy Markdown

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
Copy Markdown
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.

Comment thread packages/chart/src/FigureChartModel.ts Outdated
- 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
Copy Markdown
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
Copy Markdown
Contributor

Spacing is fine :shipit:

@mofojed mofojed merged commit 73e0b65 into deephaven:main Dec 19, 2023
@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