-
Notifications
You must be signed in to change notification settings - Fork 5
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
Callstack visualization #1922
Callstack visualization #1922
Conversation
Obtaining the call graph seems to work well. We should just restrict it to smaller parts of the code, not an entire run. For visualization I went with a We can debate very long about what the best way would be to visualize the data, but because it gets very dense very quickly I think this plot is best used interactively. I do think the color coding by script is quite nice. Some other formatting ideas are:
|
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.
Great effort! 💯
Would it be useful to depend on https://graph.makie.org/stable/generated/depgraph/? The current layout seems quite constrained, ideally you have an automatic graph layout.
For interactivity, a script in utils might be a better fit? Or a new util folder in docs, so the quarto notebook is a bit shorter?
Did you test this with a single calculation timestep as well?
Finally, given your slack question, any progress on an mermaidjs translation?
I feel like I'm converging towards something usable. I now sort per layer the names per script, leave out non-Ribasim calls (which can lead to floating branches because Ribasim calls can happen from non-Ribasim calls), and squash per layer calls with the same name into one. And an example of We could also make a distinction between different methods of a function, but including the signatures can get messy. |
Possibly all function names starting with |
Ah I forgot |
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.
Looking good 🚀. Two remarks before we merge:
- Maybe good to make some links from the equations/implementation page to each specific plot?
- In the plots all the Ribasim. prefixes should be removed.
edit:
Maybe remove the spines/axes labels/ticks with hidedecorations!
and/or the minimal theme.
xlims = (-0.4, 5.6) | ||
) | ||
``` | ||
|
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.
Maybe add the update_cumulative_flows callback here as well? That should cover most of the core code.
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 decided against adding this because update_cumulative_flows!
calls very few other (Ribasim) functions.
Fixes #1472.