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

Remove Requires and implement a Makie extension instead #181

Merged
merged 8 commits into from
Oct 8, 2024

Conversation

mohamed82008
Copy link
Member

@mohamed82008 mohamed82008 commented Oct 7, 2024

Summary

  • This PR implements Makie support as an extension instead of relying on Requires.jl.
  • I also define the visualize function now at the top level in TopOpt.jl and overload it in the extension. This means that a single visualize function is overloaded for both continuum and truss problems. This function is also exported by default.
  • This PR also avoids loading GLMakie in the extension and instead only loads and depends on Makie. Users can choose which backend they want to use themselves by loading GLMakie, WGLMakie or CairoMakie at the top level of their code.
  • Plotting code is now tested using CairoMakie as a backend on CI which is easy to setup. This is just to avoid bit rotting of code.

@mohamed82008 mohamed82008 requested a review from yijiangh October 7, 2024 05:22
@yijiangh
Copy link
Collaborator

yijiangh commented Oct 7, 2024

From @mohamed82008 's description, the intention of this PR looks good to me. But I don't understand how TopOptMakieExt gets loaded. For example, in the beso script, I don't see you import TopOptMakieExt, how does Julia dispatch visualize to the right function?

@mohamed82008
Copy link
Member Author

mohamed82008 commented Oct 7, 2024

Extension packages in Julia get loaded automatically just like Requires worked in the past. So when I call using Makie and then using TopOpt (order doesn't matter), Julia will load the TopOptMakieExt module automatically. This module extends the otherwise empty visualize function with no methods by default. So if you try to use visualize without loading Makie first, you will get an error. A nicer experience is perhaps to define a fallback that throws an informative error asking the user to load Makie:

function visualize(arg::T; kwargs...) where {T}
    throw(ArgumentError("`visualize` is not defined for input type $T. This may be because the input to the function is incorrect or because you forgot to load `Makie` in your code. You can load `Makie` with `using Makie`."))
end

@mohamed82008
Copy link
Member Author

Note that it is just a single function TopOpt.visualize. The extension module defines 2 extra methods for this function.

@mohamed82008
Copy link
Member Author

The latest commit implements a more informative error message.

@yijiangh
Copy link
Collaborator

yijiangh commented Oct 8, 2024

Extension packages in Julia get loaded automatically just like Requires worked in the past. So when I call using Makie and then using TopOpt (order doesn't matter), Julia will load the TopOptMakieExt module automatically. This module extends the otherwise empty visualize function with no methods by default. So if you try to use visualize without loading Makie first, you will get an error. A nicer experience is perhaps to define a fallback that throws an informative error asking the user to load Makie:

function visualize(arg::T; kwargs...) where {T}
    throw(ArgumentError("`visualize` is not defined for input type $T. This may be because the input to the function is incorrect or because you forgot to load `Makie` in your code. You can load `Makie` with `using Makie`."))
end

Good to know! Do we have to declare extension packages somewhere, or we simply need to put it in to the ext folder?

Makie = "ee78f7c6-11fb-53f2-987a-cfe4a2b5a57a"

[extensions]
TopOptMakieExt = "Makie"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yijiangh these 2 new Project.toml sections are where we declare the extension packages

@yijiangh yijiangh merged commit 7a67ded into master Oct 8, 2024
9 checks passed
@yijiangh yijiangh deleted the mt/makie_ext branch October 8, 2024 12:52
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