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

Export color settings in matplotlib viewers #2322

Merged
merged 7 commits into from
Oct 13, 2022

Conversation

Carifio24
Copy link
Member

Currently, the matplotlib viewers do not take into account glue's global color settings (foreground and background colors) when exporting to a Python script. This means that the exported figure will not match if the color settings differ from the default.

This PR updates the export script to take those color settings into account. The code added to the script header is partially taken from update_foreground_color and update_background_color. I've also added a test that changes the color settings, compares the exports, and resets the color settings to the default. I've placed this test in BaseTestExportPython since it's identical for each matplotlib viewer.

Also, in the course of making this update, I fixed a bug here where the wrong tick label size is used.

@dhomeier
Copy link
Collaborator

Thanks for the PR. The only thing I am wondering is, should the colours perhaps only be exported if changed from the defaults? Otherwise a session with no custom colours set will hardcode the defaults, and those will be set even if the script is run in an environment with custom settings. But maybe that is preferable for best reproducibility; also I don't see a general way to find the default values (if they should change from 'black' and 'white' at any point).

@Carifio24
Copy link
Member Author

That is a good question, of whether or not we want to respect the current glue environment if the user hasn't explicitly set foreground/background colors. I had been imagining the script as reproducing the exact view that the user had at the time of export, and so hadn't worried about that.

If we do want the script to respect the glue environment, we could import the settings from glue.config, with the colors from the current environment colors hard-coded as a fallback (e.g. for if the environment doesn't have glue installed). The only way I see to get default values is with the settings._defaults member (whose values should also just be the results of __getattr__ calls if the user hasn't made any changes). We can check if a value is equal to a default, but I don't see another way to retrieve the default values (which are set via hard-coded constants in config.py).

@dhomeier
Copy link
Collaborator

I had been imagining the script as reproducing the exact view that the user had at the time of export, and so hadn't worried about that.

I see a point in that as well, as it is primarily recreating one plot without much connection to the glue (app) configuration on that end. Not sure what the most typical use case is here.

@Carifio24
Copy link
Member Author

I agree that we should match whatever is the most typical use case. @astrofrog do you have any thoughts on which direction is more appropriate here?

@astrofrog
Copy link
Member

That's a good question - in the past we've tried to make sure the outputs match exactly. However we also want to make it easy for people to make figures look nice for including in a paper, and they might use a 'dark' mode in the UI but want to have the opposite for the exported figure. I think we do want the outputs to match by default but maybe we can reduce the amount of code in the script to be a one-liner that calls a glue function, for example set_figure_colors(background=..., foreground=...) and users can easily remove or update that line then. Does this seem like a reasonable compromise?

@Carifio24 Carifio24 force-pushed the export-color-settings branch from ea05139 to f044ae5 Compare October 11, 2022 22:10
@Carifio24
Copy link
Member Author

@astrofrog I think that's a reasonable approach. I've implemented that here - let me know if you see any issues

Copy link
Member

@astrofrog astrofrog 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, thanks!

@astrofrog astrofrog merged commit a9e1c52 into glue-viz:main Oct 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants