feat: add method to list leaf nodes #188
Conversation
src/sciline/pipeline.py
Outdated
| @@ -200,6 +205,15 @@ def _repr_html_(self) -> str: | |||
| nodes = ((key, data) for key, data in self.underlying_graph.nodes.items()) | |||
| return pipeline_html_repr(nodes) | |||
|
|
|||
| def leafs(self) -> tuple[type, ...]: | |||
There was a problem hiding this comment.
- Can we consider making this a free function? We should avoid bloating the interface of
Pipeline. - What is a leaf? Can we use some clearer terminology such as used in mathematics for DAGs, or by NetworkX? Sink nodes, for example
There was a problem hiding this comment.
What is a leaf? Can we use some clearer terminology such as used in mathematics for DAGs, or by NetworkX? Sink nodes, for example
I think sink_nodes is a better name than leaf, but they are both "leaking" the graph abstraction below Pipeline. Maybe we should have something more specific to the domain, like final_results or output_keys.
Can we consider making this a free function? We should avoid bloating the interface of Pipeline.
I'm not so sure about this. We could make it a free function, but what's the advantage of doing so? It's intrinsically tightly coupled to the Pipeline class. Having it be a method makes it easier for people to find it using dot-notation access.
src/sciline/pipeline.py
Outdated
| def final_result_keys(self) -> tuple[type, ...]: | ||
| """Returns the keys that are not inputs to any other providers.""" | ||
| sink_nodes = [ | ||
| cast(type, node) |
There was a problem hiding this comment.
Not sure I understand. What is this cast doing?
There was a problem hiding this comment.
It's just for appeasing mypy.
We promise the return type of the function is tuple[type, ...] because we know our keys are types.
But what we get from the underlying_graph is Any.
There was a problem hiding this comment.
Okay took another look now and it was actually not necessary to cast in case we just specified the return as tuple[Key, ...] instead
There was a problem hiding this comment.
Indeed. Our keys are, in general not type.
There was a problem hiding this comment.
We are using type in a lot of other places instead of Key though, that's what confused me.
For example https://github.com/scipp/sciline/blob/main/src/sciline/pipeline.py#L108-L111
Is that just a mistake or is there a difference between the contexts that makes type the right annotation there?
src/sciline/pipeline.py
Outdated
| @@ -200,6 +209,13 @@ def _repr_html_(self) -> str: | |||
| nodes = ((key, data) for key, data in self.underlying_graph.nodes.items()) | |||
| return pipeline_html_repr(nodes) | |||
|
|
|||
| def final_result_keys(self) -> tuple[Key, ...]: | |||
There was a problem hiding this comment.
Sorry about discussing names a second time, but can we think about the name once more. This seems a bit hard to remember, with the 3-part name. Have you considered output_keys? It would also be symmetric with the terminology "inputs" or input keys", which we might use elsewhere.
There was a problem hiding this comment.
No worries, yeah if you think output_keys is better we can use that instead 👍
Have you considered
output_keys?
Yes it's one of the names I suggested the first time you brought up naming 😆
Fixes #183 #177