Model analysis #939
Model analysis #939
Conversation
|
|
|
I think this is a good first step, but before coding something maybe we can simply:
In addition, this code should certainly be defined in an other class than Can this discussion be continued in the #933? |
…e + add JSON export
|
Can you share with us an example of data displayed (I think it has changed since the 1st proposal)? |
|
Here is an example of display for the Mario Kart example : |
|
And here is the associated JSON file : |
|
Once we all agree on the format, I still need to add some documentation and comments in the code to help use the ModelAnalyser (most especially as printing can be parameterised) |
|
Thank you for the trace, it is easier to get an idea on the result. I think |
| @@ -651,6 +657,13 @@ public Settings getSettings() { | |||
| return this.settings; | |||
| } | |||
|
|
|||
solver/pom.xml
Outdated
| @@ -47,6 +47,11 @@ | |||
| <artifactId>xchart</artifactId> | |||
| <version>3.8.1</version> | |||
| </dependency> | |||
| <dependency> | |||
| <groupId>com.fasterxml.jackson.core</groupId> | |||
| <artifactId>jackson-databind</artifactId> | |||
There was a problem hiding this comment.
Doesn't adding this dependency increase the size of the final jar too much?
There was a problem hiding this comment.
I think we do not need to embed the serialization inside choco-solver
There was a problem hiding this comment.
Yes, I guess we can let the user choose its own way of serializing the ModelAnalysis object.
| } | ||
|
|
||
| public void printVariableAnalysis(PrintStream ps) { | ||
| ps.println("################################################"); |
There was a problem hiding this comment.
I'm not a huge fan of these long tags.
I would prefer something more discreet :)
There was a problem hiding this comment.
What would you suggest ? The idea was to make sure it was distinguishing from other prints, but I know it might not be the tast of everyone.
There was a problem hiding this comment.
Some time ago I added a Logger attached to Solver which can either print log with ANSI colors or not.
So I suggest to add colored tags:
solver.log().green().println("Variable analysis");There was a problem hiding this comment.
I updated the ModelAnalyser printings using Solver.log(). Beginning of analysis are in green, types of constraints/variables in blue, and end of analysis in red.
| } | ||
| } | ||
|
|
||
| public static class ModelAnalyserJson { |
There was a problem hiding this comment.
I would not use "Json" in the class name.
You can serialize it into json, but you don't have to.
Also maybe ModelAnalysis is more appropriate than ModelAnalyser
There was a problem hiding this comment.
Indeed, ModelAnalysis is a more appropriate name for this class. I am changing it right away.
| /////////////////////////////////////////////// JSON EXPORT //////////////////////////////////////////////// | ||
| //////////////////////////////////////////////////////////////////////////////////////////////////////////////////// | ||
|
|
||
| private static void toFile(String path, Object toWrite) { |
There was a problem hiding this comment.
maybe this and the jackson dependency do not need to belong to choco-solver
(but would be put in the project that uses it)
Yes, given the current structure I gave to the analysis objects, the nbConstants statistics is never useful. I will remove it for now. |
|
I have added the graph model analysis, using GraphViz format. Sadly, the graphs are not always easy to read as edges might be numerous and hide nodes. |
| } | ||
| } | ||
|
|
||
| private static class Graph<T> { |
There was a problem hiding this comment.
This is redundant with https://github.com/chocoteam/choco-solver/blob/master/solver/src/main/java/org/chocosolver/solver/trace/IOutputFactory.java#L343
which should be removed in your PR. What do you think?
There was a problem hiding this comment.
Well, Gephi output should be removed too or be preferred to GraphViz (well, Gephi can read DOT files)?
https://github.com/chocoteam/choco-solver/blob/master/solver/src/main/java/org/chocosolver/solver/trace/IOutputFactory.java#L354
There was a problem hiding this comment.
The code in ModelAnalyser is a bit redundant with the one in GraphvizGenerator, but it is necessary as the one in GraphvizGenerator is inheriting from the specificities of Searchviz.
I guess that, in order to avoid redundancy in code, we could implement a generic API to generate files for graphviz and Gephi. What do you think ? Should we keep the code as it is now, or should we implement this generic API and update the two use cases ?
mergify
bot
dismissed
cprudhom’s stale review
Pull request has been modified.
|
I think we can merge this PR and use it. |
Here is a proposal for the discussion #933 on model analysis. This is a first version and can still be improved.
Among ideas to improve the analysis are the following:
Here is an example of what it does on the MarioKart example: