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

Added docs and assets for interactive simulation plotting tutorial #1053

Merged
merged 19 commits into from
Oct 16, 2024

Conversation

jonathanfischer97
Copy link
Contributor

Added tutorial on how to simulate and plot a Catalyst.jl generated ODEProblem using GLMakie.jl.

Additional embedded assets are SVG screenshots and an mp4 demoing the full interactive window.

image
image (1)
image (2)
image (3)

@TorkelE
Copy link
Member

TorkelE commented Sep 21, 2024

This looks really nice! I think my main comment is that it would be nice to run as much of the docs as possible dynamically, e.g. by using

'''@example dynamic_brusselator_example
# code
'''

rather than

'''julia
# code
'''

(see rest of docs)

That way, if there is an error in the code (of if an error is produced later on due to package updates) that is caught. I imagine this will become problems for the code doing the interactivity stuff, but some code blocks should be possible to make this way.

Copy link
Member

@isaacsas isaacsas left a comment

Choose a reason for hiding this comment

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

As @TorkelE said it would be great if you could update the first static plot to use example blocks that build it dynamically. That would at least let us catch if for some reason the beginning of the tutorial stops work.

@jonathanfischer97
Copy link
Contributor Author

Made all the requested suggestions, including dynamically building the plot from the code blocks!

The reason I didn't do this originally is because GLMakie displays the frame buffer in a separate window, so I wasn't sure how this would be displayed as an embedded plot in the HTML. But it seems there are arguments to plot inline and prevent the window, so I added that as a hidden line in the example code.

The last code block doesn't display the dynamically generated figure because I have the mp4 of the interaction instead

@jonathanfischer97
Copy link
Contributor Author

Looks like some tests and the documentation build weren't successful. This is the first time I've PRed docs, do I need to fix anything on my end?

@isaacsas
Copy link
Member

There are some issues with the Catalyst tests currently. @TorkelE is fixing them in #1070

Once that gets merged if you merge master into your local pr branch and then push it to Github that should hopefully allow your tests to go through here.

@isaacsas
Copy link
Member

(I usually use Gitkraken for such things as you can just drag master onto your local branch and tell it to merge master into your branch.)

@TorkelE
Copy link
Member

TorkelE commented Oct 1, 2024

There is something really weird going on with SI, possibly weirdest thing I've encountered. Currently faller quite sick, and don't think I will be able to do anything the next few days. I have disabled SI in the test fix PR. Please feel free to merge if we want to go ahead making updates without SI (ok for me).

@isaacsas
Copy link
Member

isaacsas commented Oct 1, 2024

@TorkelE no worries -- I hope you feel better soon!

@isaacsas
Copy link
Member

isaacsas commented Oct 8, 2024

@jonathanfischer97 can you merge master into this PR? I think that should allow tests to pass now.

@isaacsas
Copy link
Member

isaacsas commented Oct 9, 2024

@jonathanfischer97 looks like the StructuralIdentifiability extension is causing issues here still. I'll try to update master later today to completely remove it so things can work again.

@isaacsas
Copy link
Member

isaacsas commented Oct 9, 2024

@jonathanfischer97 just removed all the StructuralIdentifiability stuff if you want to merge master again. That will hopefully get this to pass CI for you.

@isaacsas
Copy link
Member

@jonathanfischer97 looks like there is a link error in the tutorial.

@jonathanfischer97
Copy link
Contributor Author

Issue with GLMakie in CI without GPU, apparently requires using xvfb in the workflow.

There's an example in the GLMakie.jl tests, will look at it next week and push the fix

- name: Install dependencies
run: julia --project=docs/ -e 'ENV["JULIA_PKG_SERVER"] = ""; using Pkg; Pkg.develop(PackageSpec(path=pwd())); Pkg.instantiate()'
- name: Build and deploy
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} # For authentication with GitHub Actions token
DOCUMENTER_KEY: ${{ secrets.DOCUMENTER_KEY }} # For authentication with SSH deploy key
GKSwstype: "100" # https://discourse.julialang.org/t/generation-of-documentation-fails-qt-qpa-xcb-could-not-connect-to-display/60988
run: julia --project=docs/ --code-coverage=user docs/make.jl
run: |
DISPLAY=:0 xvfb-run -s '-screen 0 1024x768x24' julia --project=docs/ --code-coverage=user docs/make.jl
Copy link
Member

Choose a reason for hiding this comment

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

What practical impact does this have? Does it mean all our docs are now being built under the assumption they live in a virtual 1024x768 framebuffer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the use of xvfb with -screen 0 1024x768x24 does not mean all the plots rendered during doc build will be constrained to that resolution, if that's what you mean. This setup only applies during the CI build to provide a virtual display for GLMakie plots, which require OpenGL, but also doesn't dictate the final resolution of the GLMakie plots, which instead are determined by the plot settings themselves.

Plots rendered by Plots.jl (using GR) or CairoMakie.jl aren't affected, cause these libraries use software-based rendering that doesn't depend on an OpenGL display. Their output is generated independently of the xvfb virtual framebuffer, so that those final doc artifacts are unaffected by this temporary display configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

However, if you'd rather not implement this change in the the docs CI, another option is I can just use CairoMakie as the backend for all the inline rendered plots, but make the presented code blocks look like they use GLMakie.

But this would obviously break some of the spirit of making the plots dynamically to ensure GLMakie functionality in the tutorial.

Copy link
Member

Choose a reason for hiding this comment

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

No that's fine. I just wanted to understand the code.

If tests pass is this ready to merge on your end?

@isaacsas
Copy link
Member

@jonathanfischer97 if you feel this is done I'm happy to merge it.

@jonathanfischer97
Copy link
Contributor Author

@isaacsas This is ready on my end! I think this should fix most everything, so merge at your convenience!

@isaacsas isaacsas merged commit 9debf66 into SciML:master Oct 16, 2024
4 checks passed
@isaacsas
Copy link
Member

Thanks for the PR!

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.

3 participants