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
Create a graph of all plugins. #1777
Conversation
I should also add why I put writing the output into the |
Nice visual. Great way to see everything!
Best,
-Lorraine
*****************************
Lorraine Hwang, Ph.D.
Associate Director, CIG
530.752.3656
… On May 25, 2017, at 10:10 AM, Wolfgang Bangerth ***@***.***> wrote:
Now slightly nicer with roughly equally sized blue circles:
<https://cloud.githubusercontent.com/assets/7504421/26461569/c14feb6c-413a-11e7-9f06-ec69858e43a1.png>
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub <#1777 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AESQX5k5QCWqpafatxbltTE9ANZTnJOMks5r9bYIgaJpZM4NmmB0>.
|
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.
Found the mandatory typo 😄
I like it! Makes it much easier to understand the structure of ASPECT. It is a bit of a shame that we need to introduce another boiler-plate function into every interface, but with the plugin list in an anonymous namespace I also do not see a way around it.
source/simulator/helper_functions.cc
Outdated
" style=\"filled\"];\n" | ||
"\n"; | ||
|
||
// then also write nodes for the Simulator and SimulatorAccess clsses, |
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.
classes
OK, if you think this is useful, then I will continue with this quest. |
Here is the final version of it all. It is pretty mechanical, there are really only 3 parts that are interesting:
Other than that, it's just a bunch of highly repetitive stuff. I hope to remove some of this again when merging the various |
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 looks like this is getting really big. I am not sure a simple figure in the manual will be readable. Did you think about where to put the product? Otherwise: Good job! Nearly ready.
void | ||
write_plugin_graph (std::ostream &out) | ||
{ | ||
std_cxx11::get<dim>(registered_plugins).write_plugin_graph ("Particle property interface", |
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 think this should be "Particle generator interface"
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.
Good catch!
source/simulator/helper_functions.cc
Outdated
#include <aspect/particle/interpolator/interface.h> | ||
#include <aspect/particle/output/interface.h> | ||
#include <aspect/prescribed_stokes_solution/interface.h> | ||
#include <aspect/postprocess/visualization.h> |
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.
In simulator_access.cc
we assume that all of these are included in simulator.h
and therefore do not list them explicitly. Is there a reason you want to handle this differently here?
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.
That's not quite true. For example, all of the particle headers are not included in simulator.h
, nor are the viz header file. You are correct about the prescribed_stokes_solution header, though, as well as possibly a few of the others.
In general, I think it's useful to really include in a file what is needed, rather than relying on some other header doing so transitively. What do you want me to do?
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'll add that there is essentially no cost associated with including a file twice -- the compiler caches which files it (i) has included already, and (ii) that are guarded by include guards.
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 was just curious. Let us keep it as is.
I've updated the patch. What to do with the graph: I definitely want to use it in presentations, where I can also zoom in to smaller regions. I've also thought about putting it into the manual -- not so that one can actually read (without zooming in) what the graph says, but to illustrate the architecture of ASPECT. With the infrastructure in place, it would not be very difficult to make things work in such a way that you could pass an argument to the command line that lists which subsystem you want to see. This would lead to smaller, more targeted graphs. I think some experimentation might be necessary to get this right. |
An idea that's way out there: One could imagine a tool where you get to see the graph and you can click on individual circles to enable them in your simulation. If you right clicked, you could set the parameters that correspond to this option. I'm not volunteering to write such a tool, but it would be cool way to do graphically what your GUI already does :-) |
Looks good. |
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 am a little late to the party, but I have comments/questions:
- Neat!
- Would it make sense to use the name used in the ParameterHandler section in addition to the name of the classes? I know that we don't have an automatic way to identify this name right now, but we talked about it before: automatic plugin prm subsection name #97
- I created a test: add write_plugin_graph test #1795
out << "digraph Plugins\n" | ||
"{\n" | ||
" splines=line;\n" | ||
" splines=true;\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.
why do you need two assignments to splines
? Isn't line
=false
?
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.
Good question, I don't know either, but it makes a difference and that's what the online documentation also seems to suggest one uses. Don't ask me...
"{\n" | ||
" splines=line;\n" | ||
" splines=true;\n" | ||
" overlap=false;\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.
Can you add layout=neato;
? Without it, the graph is way too wide.
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 convert the file using neato
. Adding the line doesn't seem to make any difference to me -- what are you using to create the .png?
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.
dot -Tpng bla.dot >bla.png
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.
Ah. Just use the neato
command line tool. I could also add the layout statement if you want.
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 in #1802.
On your second point: right now I use the name of the plugin that is passed to the I think the name of the plugin is the correct one to output as the primary piece of information because not all plugins actually read parameters from any section (think for example of the various -- in fact many :-) -- visualization plugins). |
Here is an attempt at addressing #1667. It adds a command line option that, when
given, initializes a Simulator object and then systematically goes through all
plugin systems and asks them to write out their part of the connection graph
for all plugins.
I haven't gone through all systems yet, just seven of them, to see whether
this is the right direction to go. It's a bit of work to convert it all,
so I thought I'd put it out there already and get a bit of feedback.