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

GraphMakie docs update #1130

Open
wants to merge 49 commits into
base: master
Choose a base branch
from
Open

GraphMakie docs update #1130

wants to merge 49 commits into from

Conversation

vyudu
Copy link
Collaborator

@vyudu vyudu commented Nov 21, 2024

Update docs and drop Graphviz dependency. Ran into an issue on my machine where ExpandTemplates was hanging for a really long time, so I changed some of the @example blocks to just ```julia, but it was still sort of hanging on the machine so not sure what's happening. In any case wanted to see how it did on the CI.

@isaacsas
Copy link
Member

isaacsas commented Nov 21, 2024

You need to also update all the places in the current docs where we show graphs to use the new code (preferably dynamically).

@isaacsas
Copy link
Member

GLMakie tests are failing?

@vyudu
Copy link
Collaborator Author

vyudu commented Nov 21, 2024

seems something GLFW-related, happening in the docs build too. Will probably debug tomorrow

@isaacsas
Copy link
Member

ok

@isaacsas
Copy link
Member

Seems like somehow the plot from Makie is getting loaded now even in tutorials that don't use Makie for plotting?

@vyudu
Copy link
Collaborator Author

vyudu commented Nov 23, 2024

seems like it was because GLMakie is loaded in that @example block, added an explicit import to fix

@vyudu
Copy link
Collaborator Author

vyudu commented Jan 6, 2025

Let's see how these look. I need to think about how to reduce the amount of whitespace though. I think it should do a bit better with Stress() but it might not be good enough.

@vyudu
Copy link
Collaborator Author

vyudu commented Jan 6, 2025

Removed most of the GLMakie instances in the docs but there's still a GLMakie instance in the interactive Brusselator doc.

@isaacsas
Copy link
Member

isaacsas commented Jan 6, 2025

There shouldn't be in an @example block, that is just in a @julia block.

@isaacsas
Copy link
Member

isaacsas commented Jan 6, 2025

(I removed all GLMakie doc dependencies last week I thought.)

@vyudu
Copy link
Collaborator Author

vyudu commented Jan 6, 2025

Check the last two commits, I changed a couple instances there? But let me know if I did something wrong

@isaacsas
Copy link
Member

isaacsas commented Jan 7, 2025

@vyudu looks like there are some text tweaks needed for the new colors / removal of labels, then this should be good.

@isaacsas
Copy link
Member

isaacsas commented Jan 9, 2025

Looks like there is a bad link in one of the new doc files?

@isaacsas
Copy link
Member

isaacsas commented Jan 9, 2025

Oh, this needs to also remove this stuff in Catalyst.jl that loads the GraphViz.jll:

# as used in Catlab
const USE_GV_JLL = Ref(false)
function __init__()
@require Graphviz_jll="3c863552-8265-54e4-a6dc-903eb78fde85" begin
USE_GV_JLL[] = true
let cfg = joinpath(Graphviz_jll.artifact_dir, "lib", "graphviz", "config6")
if !isfile(cfg)
Graphviz_jll.dot(path -> run(`$path -c`))
end
end
end
end

@vyudu
Copy link
Collaborator Author

vyudu commented Jan 9, 2025

Hmm I can't find the new bad link, it's supposedly in api.md but I don't see a [``ReactionComplex\``](@ref) in api.md.

@isaacsas
Copy link
Member

isaacsas commented Jan 9, 2025

Is it linked via a docstring?

@vyudu
Copy link
Collaborator Author

vyudu commented Jan 9, 2025

Ah I see. But in that case I don't know why it's resolving to Main or CatalystGraphMakieExtension and not looking in Catalyst.

docs/src/api.md Outdated
Comment on lines 250 to 251
Catalyst.ReactionComplexElement
Catalyst.ReactionComplex
Copy link
Member

Choose a reason for hiding this comment

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

This worked completely fine before (for example, the current docs). Why would this be the underlying issue?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm yeah this doesn't make sense. But I don't understand what could cause the error it raises otherwise, will keep looking.

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