Open jobrachem opened 6 months ago
We had a first exchange of thoughts on the matter. If we tackle the GraphBuilder, we will go through three stages:
Here are some notes from today's meeting.
@wiep @GianmarcoCallegher, my notes are included above. Feel free to comment for additions or corrections.
In the recent weeks, there have been a number of issues that are in some way related to the
lsl.GraphBuilder
class.173 (With regard to this issue, we discussed the question of what consitutes a change to a model vs. a change to a variable/node.)
167
93
88
87
I think it may be worthwile to have a look at the GraphBuilder and see how it might evolve in the future.
Three claims
To start off the discussion, I claim the following:
GraphBuilder
class is not in fact where we build the Liesel graph.GraphBuilder
instead offers methods to change an existing graph or to change nodes in that graph.GraphBuilder
is concentrated in private methods that are being used in theGraphBuilder.build_model
method.Details on the claims
Claim 1
The typical way you build a model in Liesel is by initializing variables, connecting them with calculators. Only once in this process, and most likely near the end of it, do you call the
GraphBuilder
by adding the "terminal nodes": nodes without outputs that can be used to build the whole graph by recursively following their inputs. In my usage, I often have a one-liner of the form:Claim 2
The graph- or var-manipulating methods of the graph builder are:
GraphBuilder.replace_x
(x
can be "node" or "var")GraphBuilder.rename_x
(x
can be "node" or "var")GraphBuilder.transform
Claim 3
Here's an excerpt from the
GraphBuilder.build_model
method:What follows from these claims?
For me, it currently seems like the
GraphBuilder
may not need to be its own class. I think its functionality can be allocated as follows:Model
init. As a side node, the issue addressed in https://github.com/liesel-devs/liesel/pull/167 can be solved in the process by requiring users to exclusively provide the "terminal nodes" to the model class, such that the graph is always built from the set of minimally required nodes by recursively following their inputs.GraphBuilder.replace_x
methods, as far as I can see. I am aware that the current model class is intentionally static.Concluding words
This issue is not intended as a fully worked-out solution to only be approved. It is instead intended to provide a basis for discussion about how the
GraphBuilder
might evolve.