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

Bump Makie compatibility #31

Merged
merged 2 commits into from
May 13, 2024
Merged

Bump Makie compatibility #31

merged 2 commits into from
May 13, 2024

Conversation

kbarros
Copy link
Contributor

@kbarros kbarros commented May 12, 2024

Bump Makie compatibility to include v0.21, in addition to v0.19 and v0.20.

If this is not done, having Brillouin installed in the environment will block users from upgrading to the latest release Makie.

I tested a bit the plotting support described in the documentation, and found the following to work:

using Brillouin, GLMakie

sgnum = 227
Rs = [[1,0,0], [0,1,0], [0,0,1]] # conventional direct basis
kp = irrfbz_path(sgnum, Rs)
pGs = basis(kp)      # primitive reciprocal basis associated with k-path
plot(kp) # works

However, including the Wigner Seitz cell will not work, and throws an error message "Conversion failed for Scatter [...]"

c = wignerseitz(pGs) # associated Brillouin zone
plot(c, kp) # error
Click for detailed error message
ERROR: ArgumentError:     Conversion failed for Scatter (With conversion trait PointBased()) with args: Tuple{Cell{3}, KPath{3}} .
    Scatter requires to convert to argument types Tuple{AbstractVector{<:Union{Point2, Point3}}}, which convert_arguments didn't succeed in.
    To fix this overload convert_arguments(P, args...) for Scatter or PointBased() and return an object of type Tuple{AbstractVector{<:Union{Point2, Point3}}}.

Stacktrace:
 [1] conversion_pipeline(P::Type{…}, used_attrs::Tuple{}, args::Tuple{…}, args_obs::Tuple{…}, user_attributes::Dict{…}, deregister::Vector{…}, recursion::Int64)
   @ Makie ~/.julia/packages/Makie/eziUH/src/interfaces.jl:240
 [2] conversion_pipeline(P::Type{…}, used_attrs::Tuple{}, args::Tuple{…}, args_obs::Tuple{…}, user_attributes::Dict{…}, deregister::Vector{…}, recursion::Int64)
   @ Makie ~/.julia/packages/Makie/eziUH/src/interfaces.jl:233
 [3] conversion_pipeline(P::Type{…}, used_attrs::Tuple{}, args::Tuple{…}, args_obs::Tuple{…}, user_attributes::Dict{…}, deregister::Vector{…})
   @ Makie ~/.julia/packages/Makie/eziUH/src/interfaces.jl:213
 [4] (Scatter)(user_args::Tuple{Cell{3}, KPath{3}}, user_attributes::Dict{Symbol, Any})
   @ Makie ~/.julia/packages/Makie/eziUH/src/interfaces.jl:271
 [5] _create_plot(::Function, ::Dict{Symbol, Any}, ::Cell{3}, ::Vararg{Any})
   @ Makie ~/.julia/packages/Makie/eziUH/src/figureplotting.jl:316
 [6] plot(::Cell{3}, ::KPath{3})
   @ MakieCore ~/.julia/packages/MakieCore/zeHIC/src/recipes.jl:39
 [7] top-level scope
   @ REPL[11]:1
 [8] top-level scope
   @ none:1
Some type information was truncated. Use `show(err)` to see complete types.

However, this error was already present in previous supported Makie versions v0.19 and v0.20, so does not seem to be a reason to block Makie v0.21 compatibility.

Thanks in advance!

@thchr
Copy link
Owner

thchr commented May 13, 2024

Ah, very good - thanks: let's definitely do this, yep.

(The error for plot(c, kp) is just a result of me never implementing this in the Makie interface, assuming instead that users would reach for plot(c); plot!(kp) there - but reading the docs again, which focus on PlotlyJS, I can see how this would be expected to work - I'll try to fix it)

@thchr thchr merged commit c6198a3 into thchr:master May 13, 2024
7 checks passed
thchr added a commit that referenced this pull request May 13, 2024
@thchr
Copy link
Owner

thchr commented May 13, 2024

plot(c, kp) is possible once 0.5.18 is registered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants