-
Notifications
You must be signed in to change notification settings - Fork 5
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
Set figsize to consistent 8 by 8 inches, and reduce resolution to 100 dpi #786
Conversation
32321bd
to
d88b568
Compare
d88b568
to
f61cbca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes as intended, but please consider following:
-
Evidence of different outputs between dpi=150 and default (dpi=100) on ticket would be useful. Helpful to also ensure no loss of image quality if e.g. copy/paste to ppt.
-
Would be preferable if still explicitly set dpi=(e.g.)100 in the figure settings, either so easier for local editing if user wished to change this or (preferred option) if dpi could be a user-modifiable setting in CSET. This in particular could cover use-cases where default=100dpi if not set, but dpi increases to e.g. 300dpi if user wished to generate "publication-ready" outputs.
-
Have any other approaches been considered/tested for reducing output image filesizes beyond iris/matlplotlib save defaults? Played with this a bit in ukep_plot (e.g. see https://code.metoffice.gov.uk/trac/utils/browser/ukeputils/trunk/ukep_plot/ukep_mod/plot_ukep.py#L1626), but looks like alternative methods commented out - can't recall history on this. May be useful to do some further trial and error on dpi vs filesize etc.
1Here are some plots to compare resolutions. You'll probably want to open them in a new tab, else they are going to be resized anyway. 2I'll keep the dpi argument in then. 3I've done some image optimisation experiments in #500. 100 dpi drops the filesize to 60% of the original size. I can get roughly the same savings with lossless compression, however that will make the image harder to work with (e.g, it can't be dropped straight into a PowerPoint.). The best I can do with lossless PNG is about 75% of the original file size. Reducing the resolution brings this to 50%. |
4998756
to
dfed8e0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated since first review, but being a bit pedantic on this one - sorry!
This reduces some plot resolutions to make them a bit smaller. 100 dpi is the matplotlib default.
dfed8e0
to
cd1cc75
Compare
Set plotting resolution from workflow config via rose edit. The plot resolution is then read via a function. This keeps importing the module from having side effects. Setting and getting plot resolution is tested.
I've now made the plot resolution configurable via rose edit, or a command line option if directly using the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for further updates on this one.
All looks good to me.
This reduces some plot resolutions to make them a bit smaller (800px by 800px). 100 dpi is the matplotlib default. This makes the plots load faster on the website.
Contribution checklist
Aim to have all relevant checks ticked off before merging. See the developer's guide for more detail.