search

liuyxpp / MakiePublication.jl

A Julia package for producing publication quality figures based on Makie.jl.
Other
112 stars 7 forks source link

fix: `resolution` -> `size` #12

Closed musoke closed 5 months ago

musoke commented 5 months ago

Makie v0.20.0 deprecated the resolution argument, replacing it with size 1. This causes Makie to emit deprecation warnings when a theme from MakiePublication is loaded.

Change that argument in MakiePublication too and bump the minimum Makie version to v0.20.0.

musoke commented 5 months ago

I am unsure whether this counts as a breaking change for MakiePublication since the external API does not change but version compatibility with other packages does.

codecov[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 75.96%. Comparing base (d7f491f) to head (2e51672). Report is 2 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #12 +/- ## ======================================= Coverage 75.96% 75.96% ======================================= Files 8 8 Lines 129 129 ======================================= Hits 98 98 Misses 31 31 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

liuyxpp commented 5 months ago

Thanks! Have you checked this change does not change the output of any theme? I am aware of Makie v0.20 before but do not quite understand the logic behind resolution -> size: MakiePublication's resolution is computed from figsize. I am not sure such change will invalidate figsize.

I do think it is a breaking change. So perhaps we should bump MakiePublication's version to v0.4 if this PR merges.

musoke commented 5 months ago

There isn't a change to the output.

This was a simple rename of the variable, because the Makie devs realised that they had been using the two words interchangeably. See https://github.com/MakieOrg/Makie.jl/pull/3343:

In 0.20, resolution stops being a pixel resolution in GLMakie. It was already not really true for CairoMakie before due to px_per_unit scaling. The term size is more neutral and also works better with the function resize! which we already used to change this value. A warning will now be emitted when a Scene is created while resolution is in the theme. In that case, the resolution value will be preferred over size to avoid breaking old code completely.

The only real changes are in save and Scene. In both cases, there is just a check for which keyword argument is used and a deprecation warning. Then the actual value is treated identically.