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

update docstrings for chart saving #1450

Closed

Conversation

RossKen
Copy link
Contributor

@RossKen RossKen commented Jul 18, 2023

Type of PR

  • BUG
  • FEAT
  • MAINT
  • DOC

Is your Pull Request linked to an existing Issue or Pull Request?

Closes #1441

Give a brief description for the solution you have provided

Update chart saving examples in chart docstrings

PR Checklist

  • Added documentation for changes
  • Added feature to example notebooks at tutorial in splink_demos (if appropriate)
  • Made changes based off the latest version of Splink
  • Run the linter

@RossKen RossKen linked an issue Jul 18, 2023 that may be closed by this pull request
2 tasks
@github-actions
Copy link
Contributor

Test: test_2_rounds_1k_duckdb

Percentage change: -11.4%

date time stats_mean stats_min commit_info_branch commit_info_id machine_info_cpu_brand_raw machine_info_cpu_hz_actual_friendly commit_hash
849 2022-07-12 18:40:05 1.89098 1.87463 splink3 c334bb9 Intel(R) Xeon(R) Platinum 8370C CPU @ 2.80GHz 2.7934 GHz c334bb9
1866 2023-07-18 13:46:20 1.67135 1.6603 (detached head) 9d59e56 Intel(R) Xeon(R) Platinum 8171M CPU @ 2.60GHz 2.0951 GHz 9d59e56

Test: test_2_rounds_1k_sqlite

Percentage change: -1.0%

date time stats_mean stats_min commit_info_branch commit_info_id machine_info_cpu_brand_raw machine_info_cpu_hz_actual_friendly commit_hash
851 2022-07-12 18:40:05 4.32179 4.25898 splink3 c334bb9 Intel(R) Xeon(R) Platinum 8370C CPU @ 2.80GHz 2.7934 GHz c334bb9
1868 2023-07-18 13:46:20 4.2601 4.21841 (detached head) 9d59e56 Intel(R) Xeon(R) Platinum 8171M CPU @ 2.60GHz 2.0951 GHz 9d59e56

Click here for vega lite time series charts

@NickCrews
Copy link
Contributor

Looks great to me @RossKen !

@RobinL
Copy link
Member

RobinL commented Jul 18, 2023

Whilst this looks like the best option, unfortunately it's not possible to do it this way.

The reason is that chart.save(.html) produces (I believe , but worth checking) a chart which works as a standalone web page only if you have an internet connection, whilst this function seeks to allow users in airgapped environments to use the chart.

So it needs to follow a similar approach as before, but now retrieve the chart spec from the altair chart (sorry can't remember the method for that and am on my phone)

I'm not actually sure how this interacts with the .PNG saving. I guess that probably does work offline, so the need for this function is somewhat lower, but you'd probably loose the tooltip and any other interactivity

@NickCrews
Copy link
Contributor

Ahhh, yes that makes sense. Darn.

It looks like https://github.com/altair-viz/altair_saver solves this with save(chart, "chart.html", inline=True) # HTML document with all JS code included inline?

@RobinL
Copy link
Member

RobinL commented Jul 18, 2023

Ahhh, yes that makes sense. Darn.

It looks like https://github.com/altair-viz/altair_saver solves this with save(chart, "chart.html", inline=True) # HTML document with all JS code included inline?

Good spot! - I was not aware of that. So comes down to whether we want the additional dependency. Since we already have working code, and Splink gets installed on quite a lot of strange infrastructure (often airgapped, with limited access to pypi) I'd probably err on the side of not adding the dependency, but I don't have particularly strong views

@RossKen
Copy link
Contributor Author

RossKen commented Jul 26, 2023

Given we probably don't want to add more dependencies, should we close this PR?

@RobinL
Copy link
Member

RobinL commented Jul 26, 2023

There's still a bug, it's just that now the charts are altair.Charts, to retrieve the chart as a dict you need chart.to_dict() see here rather than chart.spec

Copy link
Contributor

@ThomasHepworth ThomasHepworth left a comment

Choose a reason for hiding this comment

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

I shall rescind my approval upon reading Robin's review...

Copy link
Contributor

@ThomasHepworth ThomasHepworth left a comment

Choose a reason for hiding this comment

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

Once addressed, I'll approve
#1450 (comment)

@RobinL
Copy link
Member

RobinL commented Nov 13, 2023

@RobinL RobinL closed this Nov 13, 2023
@RobinL RobinL deleted the 1441-bug-docs-for-saving-offline-charts-are-wrong branch August 12, 2024 10:09
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.

BUG: docs for saving offline charts are wrong
4 participants