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

Julia Makie resolution depreciated. #7817

Open
agerlach opened this issue Dec 6, 2023 · 34 comments
Open

Julia Makie resolution depreciated. #7817

agerlach opened this issue Dec 6, 2023 · 34 comments
Labels
enhancement New feature or request julia
Milestone

Comments

@agerlach
Copy link

agerlach commented Dec 6, 2023

CairoMakie.update_theme!(resolution=(fig_width, fig_height))

Makie recently made a release that adds High DPI support (#2544 & #2346) which depreciates resolution in favor of size. I suspect this change will be positive for Quarto integration. See their blog post for additional details as the breaking change if more than just changing the kwarg name.

Currently, setting resolution overrides the High DPI upgrades. A depreciation warning is shown, but doesn't appear in final documents by default.

A current work around is to reset the Makie theme to the default via set_theme!()

@cscheid cscheid added the julia label Dec 6, 2023
@cscheid
Copy link
Collaborator

cscheid commented Dec 6, 2023

Thanks, that's good to know.

We probably will need to be careful. At the very least, we'd add version checks for the time being, since I don't think we can assume everyone will have instantly updated to 0.20

@agerlach
Copy link
Author

agerlach commented Dec 6, 2023

I was having some issue today reproducing results I had yesterday using GLMakie + Quarto. Running some tests I had added CairoMakie to my Project.toml but was suprised to find changes even though I didn't ever load CairoMakie. One issue with the current setup.jl is that it loads CairoMakie and changes the default theme just based on it being available in the environment.

So, when CairoMakie is not in the environment everything is fine. But when it is the theme is overridden and the Makie warning isn't shown as execute: warning is false by default. I only uncovered the issue after opening the ipynb and seeing the warning.

@cscheid
Copy link
Collaborator

cscheid commented Dec 6, 2023

I was having some issue today reproducing results I had yesterday using GLMakie + Quarto. Running some tests I had added CairoMakie to my Project.toml but was suprised to find changes even though I didn't ever load CairoMakie. One issue with the current setup.jl is that it loads CairoMakie and changes the default theme just based on it being available in the environment.

So, when CairoMakie is not in the environment everything is fine. But when it is the theme is overridden and the Makie warning isn't shown as execute: warning is false by default. I only uncovered the issue after opening the ipynb and seeing the warning.

I'm trying to understand the context here. Do I understand correctly that this is unrelated to the resolution deprecation?

@cscheid cscheid added this to the v1.5 milestone Dec 7, 2023
@agerlach
Copy link
Author

agerlach commented Dec 7, 2023

My apologies for my confusing comment.

The depreciation of resolution in favor of size is for Makie, including all backends (CairoMakie, GLMakie, WGLMakie,...). In the current release, setting resolution will ignore size and will fallback to the old implementation in order to not be breaking with the following warning

┌ Warning: Found resolution in the theme when creating a Scene. The resolution keyword for Scenes and Figures has been deprecated. Use Figure(; size = ... or Scene(; size = ...) instead, which better reflects that this is a unitless size and not a pixel resolution. The key could also come from set_theme! calls or related theming functions.
└ @ Makie ~/.julia/packages/Makie/6NLuU/src/scenes.jl:220

Note how Quarto's setup.jl updates the theme to set the resolution

CairoMakie.update_theme!(resolution=(fig_width, fig_height))

This line is contained in a try-catch block that calls import CairoMakie.

So, if a user uses the following in Quarto, the output will actually depend on if they have CairoMakie available in the environment.

using GLMakie # NOTE NOT CairoMakie

f = Figure(size = (100,100))

If CairoMakie is not available the try block fails on import CairoMakie and the theme is not updated w/ resolution. However, if CairoMakie is available then the theme is updated regardless of if the user explicitly loads it and all plots will ignore size and fallback to the old depreciated implementation. This happens silently because the depreciation warning is not included in the final output since execute: warning is false by default in Quarto.

I will try to put together a MWE that illustrates this.

@cscheid
Copy link
Collaborator

cscheid commented Dec 7, 2023

I understand you now.

Since you have more experience with Makie than we do, would you be willing to write a PR that addresses these issues in a way that works for 0.20.0 and doesn't for 0.19.0? I'd be more than happy to help you on the Quarto side.

@agerlach
Copy link
Author

agerlach commented Dec 7, 2023

MWE. no_cairomakie.pdf and with_cairomakie.pdf shows the results without and with CairoMakie in the environment, respectively.

---
title: Title
jupyter: julia-1.9
# execute: 
#   warning: true
format: revealjs
---

## Status

```{julia}
using Pkg
cd(@__DIR__)
Pkg.activate("."; io=devnull) 
# Pkg.instantiate(; io=devnull)

using GLMakie
x = rand(50)
Pkg.status()
```

## 

:::: {.columns}

::: {.column width="50%"}
### Quarto Theme
```{julia}
f, ax, p = lines(x; axis = (; title="Title", xlabel = "X label", ylabel = "Y label"),
  label = "data")
axislegend(ax)
f
```
Plotting w/o touching the Makie theme.
:::

::: {.column width="50%"}
### Default Makie Theme
```{julia}
set_theme!() # reset theme to default. Reverses Quarto setup.jl
f, ax, p = lines(x; axis = (; title="Title", xlabel = "X label", ylabel = "Y label"),
  label = "data")
axislegend(ax)
f
```
Resetting the Makie theme to Makie's default
:::
::::

no_cairomakie.pdf
with_cairomakie.pdf

@agerlach
Copy link
Author

agerlach commented Dec 7, 2023

would you be willing to write a PR that addresses these issues in a way that works for 0.20.0 and doesn't for 0.19.0? I'd be more than happy to help you on the Quarto side.

I am happy to help. I have an idea of how to handle the various versions, but it would probably be a hack. The fact that 0.20.0 now interprets the size

of a Figure in device-independent pixels like CSS, we can display png images with the text/html MIME type, annotated with the original size via the width and height HTML attributes. (blog post)

B/c of this there may be a much better way to integrate Makie w/ Quarto. I will ping the Makie devs on Julia's slack to this issue to try to get their thoughts.

@cscheid
Copy link
Collaborator

cscheid commented Dec 7, 2023

it would probably be a hack

We earnestly do not mind hacks at all in Quarto as long as the improvement in user experience is worth it (and I agree with you that getting good high-DPI support would be 100% worth it!)

@mcanouil
Copy link
Collaborator

mcanouil commented Dec 7, 2023

Side note:

This happens silently because the depreciation warning is not included in the final output since execute: warning is false by default in Quarto.

This is a bit inaccurate.
When using format: revealjs or any presentation format, both echo: false and warning: false because the focus of a presentation is usually not the code.

When using other formats such as html everything is set to true, so if warnings are emitted they are displayed.

---
title: Title
jupyter: julia-1.9
format:
  revealjs: default
  html: default
---

```{julia}
@warn "This is a warning message"
```

@agerlach
Copy link
Author

agerlach commented Dec 7, 2023

Welp, I spent most of the day getting a working PR put together. However, unfortunately, the required corporate contributor agreement is problematic. The time and energy that it will take, from both our sides, will far outweigh the value of my contribution. If you see another path forward, let me know.

@cscheid
Copy link
Collaborator

cscheid commented Dec 7, 2023

Totally understood, and I apologize, I should have let you know ahead of time. It's not something we control on our end either.

@jkrumbiegel
Copy link
Contributor

I can't tell what's going wrong here, what are the actual theme values for resolution that quarto sets for you? Does the wide image actually correspond to that? In principle, setting resolution should still work as before, only the effective pixel size of the output changes because the px_per_unit value is higher

@cscheid
Copy link
Collaborator

cscheid commented Dec 8, 2023

@jkrumbiegel (first, thanks for Makie!) By default, Quarto calls

CairoMakie.update_theme!(resolution=(fig_width, fig_height))

Where fig_width and fig_height are defaults for figure widths and heights in inches.

I'm not sure what @agerlach originally asked, but as a Quarto dev, I can tell you that what we'd like to be able to offer is "as-good-as-possible" defaults across the many different formats and configurations that Quarto covers. It seems that Quarto should be setting size and px_per_unit instead of resolution.

Final question -- and I apologize -- How should we check for the version of CairoMakie? We'd like to do the right thing for v0.20, but not break in older installs. I'm assuming the recommended way is to iterate through Pkg.dependencies() to find the correct PackageInfo entry, but I'm not an experienced Julia dev. Is there a more idiomatic way?

@agerlach
Copy link
Author

agerlach commented Dec 8, 2023

Totally understood, and I apologize, I should have let you know ahead of time. It's not something we control on our end either.

@cscheid I certainly understand why its needed. I pinged the Makie and Quarto users on Julia slack to the issue. jkrembiegel is one of the Makie devs.

I can't tell what's going wrong here, what are the actual theme values for resolution that quarto sets for you? Does the wide image actually correspond to that?

@jkrumbiegel The default values used by Quarto are dependent on the document format. See the defaults here. The pdfs I posted above were prints of a "HTML Slides (reveal.js)" which defaults to 9 x 5.

Quarto injects a "startup cell" into the intermediate jupyter notebook that runs https://github.com/quarto-dev/quarto-cli/blob/a2b6663ad987e01e0ae9f13206b8017857052ced/src/resources/jupyter/lang/julia/setup.jl

which sets type and resolution IF CairoMakie is able to be loaded. The left figure is using the theme set by Quarto while the right is after resetting to the default via set_theme!(). Both of these plots were generated with GLMakie.

try
import CairoMakie
# CairoMakie's display() in PDF format opens an interactive window
# instead of saving to the ipynb file, so we don't do that.
# https://github.com/quarto-dev/quarto-cli/issues/7548
if fig_format == :pdf
CairoMakie.activate!(type = "png")
else
CairoMakie.activate!(type = string(fig_format))
end
CairoMakie.update_theme!(resolution=(fig_width, fig_height))
catch e
# @warn "CairoMakie init" exception=(e, catch_backtrace())
end
# Set run_path if specified
try
run_path = raw"{4}"
if !isempty(run_path)
cd(run_path)
end
catch e
@warn "Run path init:" exception=(e, catch_backtrace())
end

image

However, if CairoMakie is not available in the stacked environment the setting is not overridden. This is generated using the exact same Quarto code but w/ CairoMakie removed from the environment.
image

So, I see a couple of issues.

  1. startup.jl ends up overriding the theme using the depreciated approach.
  2. Setting the Quarto theme is dependent on CairoMakie being available. I think it may be a better idea to check if Makie's UUID is in deps = Pkg.dependencies()
  3. Setting the theme requires loading CairoMakie. Instead, can the theme be set directly with Makie? I realize that most users would not have Makie as a direct dependency in their environment, but one could search deps for any direct dependency w/ Makie as a sub-dependency. If we call that dep parent_pkg the setup.jl could import Makie directly via import parent_pkg.Makie. This would essentially build in support for any current or future backend.
  4. deps[makie_UUID].version could be used to check if pre v0.20.
  5. There is currently some logic for the figure type that may be OBE by recent Makie releases.
  6. For HTML outputs it seems like Quarto should be exploiting display png via html preserving size regardless of pixel density MakieOrg/Makie.jl#2346. I'm not quite sure what that looks like though.

@dmbates
Copy link

dmbates commented Jan 12, 2024

Have there been any further developments on this issue? The Quarto/CairoMakie combination is important to me and I would be happy to help in whatever way I can to come up with workarounds, etc.

@cderv
Copy link
Collaborator

cderv commented Jan 15, 2024

Have there been any further developments on this issue?

Not yet as this is scheduled for 1.5 as you can see from the milestone on the right. We'll update here where there is.

I would be happy to help in whatever way I can to come up with workarounds, etc.

Thanks a lot for offering your help. We welcome PR, provided that a contributor agreement can be signed. (https://github.com/quarto-dev/quarto-cli/blob/main/.github/CONTRIBUTING.md#to-submit-a-contribution-using-a-pull-request)

We'll get to 1.5 issues, once 1.4 is released, which should be soon.

@dmbates
Copy link

dmbates commented Jan 16, 2024

To follow up on some of the points made by @agerlach and @jkrumbiegel

  • Most projects do not depend on Makie directly but instead depend on one of the backends such as CairoMakie or GLMakie.
  • Any project with a direct dependency on, say, CairoMakie will have an indirect dependency on Makie. Thus setup.jl can check if there is any need to adjust the default theme for Makie by code like
julia> import Pkg, UUIDs

julia> makie_uuid = UUIDs.UUID("ee78f7c6-11fb-53f2-987a-cfe4a2b5a57a")
UUID("ee78f7c6-11fb-53f2-987a-cfe4a2b5a57a")

julia> deps = Pkg.dependencies();

julia> haskey(deps, makie_uuid)  # is the Makie package available?
true

and establish the version as

julia> deps[makie_uuid].version
v"0.20.4"
  • If Makie is a dependency it may not be a direct dependency
julia> deps[makie_uuid].is_direct_dep
false

in which case, as far as I know, you can't import it or declare using Makie. Thus you can't call Makie.update_theme! directly.

So, if Makie is a dependency, I think the best course is to iterate over the UUIDs for the backends like CairoMakie or GLMakie and check if they are in the dependencies with is_direct_dep being true, in which case you import the backend and update the theme.

If @jkrumbiegel and @agerlach think this approach makes sense I could create a PR or just describe the approach in a bit more detail so that a PR can be created at Posit.

@juliohm
Copy link

juliohm commented Feb 7, 2024

This issue is affecting our book written in Quarto.

See the visualizations in this chapter: https://juliaearth.github.io/geospatial-data-science-with-julia/02-geoviz.html

@dmbates
Copy link

dmbates commented Feb 7, 2024

@juliohm A temporary fix to avoid warnings appearing in the output is to add

#| warning: false

to the code blocks creating the figures. Of course, a long-term solution is still needed.

@juliohm
Copy link

juliohm commented Feb 7, 2024

Thank you @dmbates , I agree. I am positive that Quarto maintainers are taking a look into it 🙏🏽

@juliohm
Copy link

juliohm commented Mar 19, 2024

This issue has gotten worse. Now I cannot update my Quarto book because the Makie figures look wrong:

image

I tested outside Quarto and everything looks correct.

Can someone from the Quarto team please solve this issue?

@cscheid
Copy link
Collaborator

cscheid commented Mar 19, 2024

@juliohm That doesn't look like the same issue to me. I can't see how a DPI setting could cause a mangled output like that. Can you open a new issue? Thank you.

@juliohm
Copy link

juliohm commented Mar 19, 2024

@cscheid I can't see it either. It is hard to isolate the issue given that it is getting worse with time.

Can this issue be considered a high-priority bug? It was opened > 3 months already, and is starting to compromise our ability to deliver educational content as end-users of Quarto + Julia.

@cscheid
Copy link
Collaborator

cscheid commented Mar 20, 2024

Sorry, can you say more about how this specific bug is compromising your ability to deliver content? I understand that we'd like to fix the DPI issues, but I currently don't estimate this bug to be more urgent than the other bugs that we are handling on a daily basis.

As always, this is an open source project, and if you have the time and willingness to contribute an appropriate fix, we'll be happy to take it.

(WRT your other bug, I don't see how it could be related to the deprecation of resolution; I do agree that the figure is broken so if we can isolate the cause, we'll make sure to fix it in a timely manner)

@juliohm
Copy link

juliohm commented Mar 20, 2024

Sorry, can you say more about how this specific bug is compromising your ability to deliver content?

We cannot update the book, nor trigger the CI to update the deployed book online. There are chapters and improvements in the works, and we are currently unable to push these additions because it will break the whole set of visualizations already deployed.

@juliohm
Copy link

juliohm commented Mar 20, 2024

Can you share the high-level steps to fix this issue? We have Julia skills, and can try to find people with JS/TS skills to help.

@cderv
Copy link
Collaborator

cderv commented Mar 20, 2024

Can you share the high-level steps to fix this issue? We have Julia skills, and can try to find people with JS/TS skills to help.

I believe @agerlach shared its investigation first at #7817 (comment) and then on this comment #7817 (comment) showing what is currently done in setup.jl

This bug here is related to resolution deprecation. Your issue with broken figure may be different and it would help having a reproducible example of that broken figure to investigate in case this is different issue.

Hope this helps. Thanks for considering contributing a PR !

@agerlach
Copy link
Author

agerlach commented Mar 20, 2024

@juliohm I've dug into this issue a bit and I have a hard time imagining that what you are experiencing is related to my OP. I do have some suggestions for you to test

  1. add set_theme!() right after loading Makie. This essentially reverts all Makie settings injected by Quarto. If that doesn't fix it, then it is completely unrelated to this issue and you should create a new issue.
  2. Try removing everything after line 18 in quarto/share/jupyter/lang/julia/setup.jl (this is the local location of the script mentioned in OP)
  3. Keep your ipynb when running Quarto via https://quarto.org/docs/computations/execution-options.html#intermediates and check the resulting notebook. Is the issue still present in the notebook? If so, its a Makie / Ijulia problem and not a Quarto problem

I originally made a PR that address the OP, HOWEVER, contributing to this project requires signing legal documents that would be doable, but extremely painful given the size of my organization. I would encourage you to look at my fork/PR for inspiration.

@agerlach
Copy link
Author

@cscheid @cderv The more that I've thought about this issue the more I am convinced that Quarto is trying to do too much with respect to plotting in setup.jl. My primary issue with it is that it loads Plots/ Makie simply if it is a users environment even if they don't want it loaded.

I would prefer setup.jl to simply define the following and put the onus on the user to configure the size of whatever plotting package they are using. This can be aided via documentation.

IJulia.clear_history()

fig_width = {0}
fig_height = {1}
fig_format = :{2}
fig_dpi = {3}

# no retina format type, use svg for high quality type/marks
if fig_format == :retina
  fig_format = :svg
elseif fig_format == :pdf
  fig_dpi = 96
  # Enable PDF support for IJulia
  IJulia.register_mime(MIME("application/pdf"))
end

@cscheid
Copy link
Collaborator

cscheid commented Mar 20, 2024

@agerlach understood. I'm going to have to discuss this with the team because it will be a breaking change to our previous behavior, but what you're saying makes sense.

@juliohm
Copy link

juliohm commented Mar 20, 2024

  1. add set_theme!() right after loading Makie. This essentially reverts all Makie settings injected by Quarto. If that doesn't fix it, then it is completely unrelated to this issue and you should create a new issue.

I can confirm that this does not fix the issue, probably unrelated then.

@juliohm
Copy link

juliohm commented Apr 11, 2024

The issue reported above where the figure is displayed with stripes is caused by a new version of Cairo_jll. I am fixing Cairo_jll at v1.16 now to get the correct plots. I don't know why this only happens in Quarto though.

@jkrumbiegel
Copy link
Contributor

@juliohm are you using svg or png? I assume svg since png is a binary format. In some modes, quarto or pandoc are modifying svg files to circumvent certain issues like conflicts between assets on a page. I filed an issue about such a bug some weeks ago which was fixed in pandoc but isn't yet in released quarto. Might be that the new Cairo version outputs slightly different svg code that surfaced another bug in this transformation code. I would cross check the svg code of a broken svg using quarto with the same svg rendered outside of quarto.

@juliohm
Copy link

juliohm commented Apr 11, 2024

@juliohm are you using svg or png?

We use SVG. Only one chapter uses PNG because of the large number of figures.

@mcanouil mcanouil added the enhancement New feature or request label Apr 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request julia
Projects
None yet
Development

No branches or pull requests

7 participants