Closed jkormu closed 3 years ago
Thank you very much - having this visualization directly into Ceres will be really cool!
It's impressive also for your first C# code and also that you figured out some of the relatively undocumented Ceres internals.
I don't think any substantive changes would be needed, just a number of smaller stylistic items. Comments below.
It's great that you leveraged System.Drawing.Common for this functionality because Microsoft built it to be cross-platform, also working on Linux. https://www.hanselman.com/blog/how-do-you-use-systemdrawing-in-net-core Although it requires a sudo install on Linux of libc6-dev and libgdiplus, this should not generally be a problem (in worst case, this functionality would simply not work).
Ceres consists of a stacked set of assemblies at different layers. The Ceres.MCTS layer is intended to contain only logic relating to MCTS search. However this tree plot is clearly at the presentation/feature layer, so I suggest creating a folder Visualization in the Ceres.Features assembly and putting this class in a file in this folder with namespace Ceres.Features.Visualization.TreePlot (or the like).
In UCI code, the output should be sent to OutStream instead of Console
By convention, each class is put in its own file of the same name. Also generally the public fields have the first characater capitalized and appear first, followed by the more private fields (with names that are all lower case).
I'm trying to follow the convention that braces are always used blocks (e.g. if, for, while, using) even if they are a single line.
It would be cool if the image viewer would automatically launch and show the generated png. This can be done using something like Process.Start("plot.png") but this is not cross-platform (I forget the right way to do this that also would work on Linux).
By convention comments are terminated with a period (already mostly the case).
The code base is currently all "2 spaces" (this option can be set in the Visual Studio preferences).
This code can be simplified from:
return !(Thread is null) ? Thread : (Children.Count > 0 ? Children[0] : null);
simpler:
return Thread is not null ? Thread : (Children.Count > 0 ? Children[0] : null);
simplest:
return Thread ?? (Children.Count > 0 ? Children[0] : null);
var children = (from ind in Enumerable.Range(0, node.NumChildrenExpanded) select node.ChildAtIndexRef(ind)).ToArray();
However in some cases, it can be inefficient. This is almost certainly the case because here because it ends up materializes the data structure (ToArray()). Instead, the spririt of Linq is to just keep enumerating over chains and then everything is "one at a time, just in time."
It seems you could do this here, just chain this all together in the foreach appearing below:
foreach (MCTSNodeStruct child in children.OrderBy(c => -c.N))
{
}
then it there will be less objects created and speed will be much better.
Just a thought - perhaps you could add not only the current save-tree-plot command but also a show-tree-plot command that does the same thing but auto-launches the png in the visualizer application, to make it really easy. We could also suggest to the author of Nibbler to add a menu choice to launch that.
Thank you for the detailed comments! I will address those.
I have only couple one questions.
Ceres consists of a stacked set of assemblies at different layers. The Ceres.MCTS layer is intended to contain only logic relating to MCTS search. However this tree plot is clearly at the presentation/feature layer, so I suggest creating a folder Visualization in the Ceres.Features assembly and putting this class in a file in this folder with namespace Ceres.Features.Visualization.TreePlot (or the like).
I initially tried this but couldn't figure out how to refer to namespace defined in Ceres.Features from Ceres.MCTS. It seems that one can only access namespaces of the Layers that are defined as project dependencies for the current Layer. However, I can't add Ceres.Features as dependency for Ceres.MCTS as it would result in circular dependency (Ceres.Features depends on Ceres.MCTS already). Any advice?
EDIT: realized that the only reason why I had this problem was that during development I was calling TreePlot.Save directly from the search code. But now that it is called from UCI, it is all fine as UCI resides in Features layer.
I'm trying to follow the convention that braces are always used blocks (e.g. if, for, while, using) even if they are a single line.
Could you give an example of bad and correct way? Tried to compared how I and rest of the code base use if and while blocks and couldn't spot the difference.
Rest seems clear for now.
I also had show-tree-plot idea in my mind but ditched it as it seemed like I would have to use Windows Forms which sounded like it will never work on Linux. I will look into the suggested way of using Process.Start("plot.png").
Actually the use of braces is almost perfect, just I noticed not here (missing):
using (new SearchContextExecutionBlock(curContext))
TreePlot.Save(curManager.Context.Root.Ref, fileName);
I hope I got it all.
Adds UCI command
save-tree-plot <filename>
to save visual presentation of current search tree as a png-file.Example UCI session
This saves following plot into file C:\temp\myplot.png.
Works well for larger trees, as long as memory does not run out. Drawing is fairly quick. On my machine with debug build plotting 1M node tree takes about 10 seconds, ~2 sec to calculate the x,y-coordinates and ~8 sec to draw it. Smaller trees are much faster to plot, e.g. 8000 nodes takes ~0.1 sec.
Some use cases:
Some potential future improvements:
This is my first time writing C# and implementation might be ugly at places. Happy to hear feedback how to improve.