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 hv.save for png images #4304

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix hv.save for png images #4304

wants to merge 2 commits into from

Conversation

Guillemdb
Copy link

Closes #4302

Calling holoviews.save on panels ignores the parameter fmt and outputs all the layout objects as html files.

This PR updates the name of the file passed to plot.layout.save to include the file extension when the selected format is png. This allows panel to also export the panels as png images.

@Guillemdb
Copy link
Author

Guillemdb commented Mar 19, 2020

AFAIK panel only supports html and png formats, so I added a check for the format to be value. If you want invalid formats to be handled differently please tell me and I will update it.

I am not familiar with the tests of the package. But If you point me to the place where you are testing the function I modified I can add a test to assert the ValueError is raised.

I can also change the message of the error if you have a better suggestion.

@philippjfr
Copy link
Member

To be clear the intention was that the format would be specified as part of the filename and fmt would mostly be used to switch between different widget types, i.e. widgets or scrubber.

@philippjfr
Copy link
Member

I think hv.save should probably check the filename extension and then validate that against the supplied format but that can be a separate PR.

@Guillemdb
Copy link
Author

To be clear the intention was that the format would be specified as part of the filename and fmt would mostly be used to switch between different widget types, i.e. widgets or scrubber.

I have been using this wrong all the time 😅, I will modify the code of my libraries to reflect this. Thanks for the tip!

@philippjfr
Copy link
Member

I have been using this wrong all the time 😅, I will modify the code of my libraries to reflect this. Thanks for the tip!

That's likely more of a failure of our docs, if you have suggestions on how and where to clarify this I'd be very open to make those changes.

@Guillemdb
Copy link
Author

I think hv.save should probably check the filename extension and then validate that against the supplied format but that can be a separate PR.

Should I remove the ValueError?

If you send me a link with the valid formats (just to be sure I am not missing anything) I can make another PR with that.

@philippjfr
Copy link
Member

Should I remove the ValueError?

I think the ValueError is fine tbh, that's all the formats that bokeh can export directly. Anything else is already handled in a different way.

@Guillemdb
Copy link
Author

That's likely more of a failure of our docs, if you have suggestions on how and where to clarify this I'd be very open to make those changes.

I will think about it and make an informed suggestion (I hope in the form of a PR)

@jbednar jbednar added the type: docs Related to the documentation and examples label Nov 20, 2020
@jbednar jbednar added this to the v1.14.x milestone Nov 20, 2020
@philippjfr philippjfr modified the milestones: v1.14.x, v2.0 May 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: docs Related to the documentation and examples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Layouts are not correctly exported
3 participants