-
Notifications
You must be signed in to change notification settings - Fork 6
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
Run notebook examples in CI #132
Conversation
Move OrdinaryDiffEq to [extras] and use TestEnv to add it for examples Update gitignore to ignore all ipynb_checkpoints & rm existing ones in repo Restore Graph function for StockAndFlow Remove non-existing exports
…into notebooks-regression-test
Remove Pkg.activate("../../..") ClOUD -> CLOUD CausalLoop uses vectors now, not tuples
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks Alex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any particular reason you changed it to a Vector? I'd think it'd work either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't work either way.
MethodError: no method matching CausalLoop(::Tuple{Symbol, Symbol, Symbol}, ::NTuple{4, Pair{Symbol, Symbol}})
Stacktrace:
[1] top-level scope
@ In[2]:2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh dang. Sorry.
@@ -663,7 +661,7 @@ | |||
" \n", | |||
" :flows\n", | |||
" CLOUD => f_NewBorn(v_NewBorn) => NormalWeight\n", | |||
" NormalWeight => f_DeathNormalWeight(v_DeathNormalWeight) => ClOUD\n", | |||
" NormalWeight => f_DeathNormalWeight(v_DeathNormalWeight) => CLOUD\n", | |||
" NormalWeight => f_BecomingOverWeight(v_BecomingOverWeight) => OverWeight\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I swear, I thought we've fixed this same thing 3 times over...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we have CI on these, hopefully it never slips back in! 😃
Just a remark that the “official” way to accomplish what you’re doing here is with Literate.jl, which will generate Documenter files, notebooks, and scripts from the same markdown source, if you find that shell script inconvenient to maintain at some point. |
Issue
Our notebook examples easily go out of date, and it's hard to notice when it happens without a regular CI checking them
Fix
Convert notebook examples to Julia code and run them
Fix errors CI exposed:
Graph
was removed fromvisualization.jl
, but was used by examples, especially older onesClOUD
->CLOUD
Graph
->GraphCL
Pkg.activate('../../../')
in a causal loop exampleNotes
Includes commits from #131 and verifies removing
GraphViz
did not break any examples. @KevinDCarlsonAlso addresses #113 (comment)
Misc
Aqua.jl
around non-existent exportsfull_fledged_schema_examples/CaulsalLoopDiagrams
tofull_fledged_schema_examples/CausalLoopDiagrams
These tests can be run locally by running the helper script,
validate-notebook-examples.sh
, which does what the workflow does:TODO
Graph
and what our 'display' API looks like going into a full initial 1.0Threads.@threads
on the for loop in the test runner, but was getting strange errors:Reason: LoadError: package StockFlow did not define the expected module StockFlow, check for typos in package module name