search

JuliaPlots / PlotlyJS.jl

Julia library for plotting with plotly.js
Other
422 stars 78 forks source link

kaleido/savefig for any objects outside PlotlyJS.jl #435

Closed joshday closed 1 year ago

joshday commented 2 years ago

It appears (https://github.com/JuliaComputing/PlotlyLight.jl/issues/8) that PlotlyJS.savefig works out of the box for PlotlyLight.Plots if you simply remove the type annotations in the savefig code.

How would ya'll feel about one of these options?

  1. Put the savefig code in its own package. Then PlotlyJS wouldn't need to depend on Kaliedo_jll.
  2. Remove the type annotations in the savefig code, so other Plotly packages could take advantage of it.
sglyon commented 2 years ago

I’d be happy to get this working. Perhaps it belongs in Kaliedo package?

can you elaborate more on what you were thinking with (1)

joshday commented 2 years ago

can you elaborate more on what you were thinking with (1)

Please correct me if I'm mistaken, but I think we can leave off the ::Plot type annotation

https://github.com/JuliaPlots/PlotlyJS.jl/blob/562082f9a584a22bd9bda42dc0402640b5e8f611/src/kaleido.jl#L73-L79

Then as long as p gets JSON.json-ed into the right format, everything should "just work". https://github.com/JuliaPlots/PlotlyJS.jl/blob/562082f9a584a22bd9bda42dc0402640b5e8f611/src/kaleido.jl#L86-L92

(as it happens, a PlotlyLight.Plot gets JSON.json-ed into the right format)

I could also imagine a savefig method that accepts the JSON string, e.g. 

fig_string = """{"data": ...}"""

savefigfromjson(fig_string)

and then the yet-to-be-created Kaleido package wouldn't need to depend on JSON/JSON3. It's more verbose though, so it depends on what you want with that tradeoff.

savefigfromjson(JSON.json(myplot))

savefigfromjson(JSON3.write(myplot))
disberd commented 2 years ago

Somehow related to https://github.com/JuliaPlots/PlotlyJS.jl/issues/428

BeastyBlacksmith commented 2 years ago

I'm much in favor of having a seperate package for saving plotly plots to different output formats. Soft-pinning PlotlyBase to 0.7 in Plots is beginning to hurt and I'd rather avoid having Kaleido_jll as a direct dependency.

sglyon commented 2 years ago

@BeastyBlacksmith happy to consider something here. What would you propose?

BeastyBlacksmith commented 2 years ago

I'm with @joshday here to create a Kaleido.jl package that takes a json-string and the desired output format and a target path and then creates a file.

joshday commented 2 years ago

Any volunteers to create it? I'll get to it next week (probably) if there are no takers.

BeastyBlacksmith commented 2 years ago

I don't know much about the package, but if its just copying the stuff from src/kaleido.jl in a seperate package, I could also do that

sglyon commented 2 years ago

Yeah I think all the code is out there. Copy/paste to move it to a separate package should be the way forward.

The only potential issue is deciding how dependencies flow. Will PlotlyJS.jl depend on Kaleido.jl? If so, what types will be used for the savefig methods in the Kaleido.jl source (if you look at src/kaledio.jl you see we are using types from both PlotlyJS.jl and PlotlyBase.jl)

BeastyBlacksmith commented 2 years ago

Will PlotlyJS.jl depend on Kaleido.jl? If so, what types will be used for the savefig methods in the Kaleido.jl source (if you look at src/kaledio.jl you see we are using types from both PlotlyJS.jl and PlotlyBase.jl)

I'd say Kaleido.jl should be agnostic to any types and just work on the string representation of the plot. So I'd change that and then PlotlyJS.jl would convert its Plot objects to string before passing it to Kaleido.jl.

BeastyBlacksmith commented 2 years ago

I made a first version here: https://github.com/JuliaPlots/Kaleido.jl