Should `eval_graph` have an `input=:A` kwarg?

[mfasi]

The function eval_graph in eval.jl implicitly assumes that the node corresponding to the input node has label :A. Some of the graph generators, such as for example ps_poly.jl and monomial_poly.jl for instance, allow the user to use a different label via the kwarg input.

I'd be inclined to add the input kwarg to eval_graph if nobody sees an issue with that.

[jarlebring]

In principle, yes. Is there something specific this would simplify/solve? The reason some graph generators have an input is that we wanted to do composition of graphs, and then it was easier to combine if we had control of the input-variable. Another option is maybe to add a input to the Compgraph-struct, depending on what you want to improve.

[mfasi]

In principle, yes. Is there something specific this would simplify/solve?

I'm not sure yet. I noticed this only because I was testing rename_node!() via eval_graph() and was getting some unexpected errors when the input label was changed from :A to something else.

The reason some graph generators have an input is that we wanted to do composition of graphs, and then it was easier to combine if we had control of the input-variable.

The renaming function came about while I was working on something related to this. As far as I can tell, whenever one uses merge_graphs the resulting graph may in principle have redundant nodes (same operation and same parents, just different label), but at the moment we don't have a function that identifies all instances of such nodes and merges them together.

I realized this while I was trying to use merge_graphs to write the short form of pade.jl you suggested in #1. The function gen_rational implements the pade.jl-like generator in this way, but for powers of the input matrix it typically produces redundant nodes that the version on Dropbox didn't have.

My plan was to write a compression function for redundant nodes, but in order to avoid mistakenly identifying labels corresponding to different input matrices it is necessary to have a mechanism to specify the correspondence between input nodes of the two graph.

Another option is maybe to add a input to the Compgraph-struct, depending on what you want to improve.

We could simply write a function that looks at the graph and identifies all the input nodes (they appear as parent but don't have parents themselves). Having an array of input nodes in the Compgraph type would be a neater way of doing it, but it might require a bit more work as some of the other functions in compgraph_struct.jl and compress.jl (and probably in other files too) would have to be modified accordingly.

I'm happy to work of this and follow whatever solution you think is more reasonable.

[eringh]

Good point about gen_rational ! I was trying a slim approach with code resuse, but maybe it became to naive.

Perhaps we could pass some type of dictionary/vector as a kwarg to merge_graphs , that identifiy nodes that are the same in both graphs. It will not solve the general case of redundancy, but if merge_graphs is the main source maybe it is easier to fix it there.

[jarlebring]

Actually, I don't understand. I see you have added rename_node! in compgraph_struct.jl. How is that implementation different from the old rename_node! function? See this code line: https://github.com/jarlebring/GraphMatFun.jl/blob/e7e2031d823f3bf7ec7569c35ac58578ec201d11/src/compgraph_struct.jl#L130
I'm also confused since Emil has already ticked the pade version in #1, so I'm not sure which feature you are referring to. Some misunderstanding?

[mfasi]

Good point about gen_rational ! I was trying a slim approach with code resuse, but maybe it became to naive.

I had the same idea in mind, just wanted to add an additional step.

Perhaps we could pass some type of dictionary/vector as a kwarg to merge_graphs , that identifiy nodes that are the same in both graphs. It will not solve the general case of redundancy, but if merge_graphs is the main source maybe it is easier to fix it there.

I think that having something like that in merge_graphs would be very useful indeed. Even if we decide to integrate compress_graph_redundant! (see below) into merge_graphs, we could still use something like this to tell the function what input nodes of the two graphs being merged should be identified.

Actually, I don't understand. I see you have added rename_node! in compgraph_struct.jl. How is that implementation different from the old rename_node! function?

It wasn't. I hadn't seen that function just a few lines above. I removed that second implementation with bce4ba3, but I kept the new tests I had added.

I'm also confused since Emil has already ticked the pade version in #1, so I'm not sure which feature you are referring to. Some misunderstanding?

I think it just so happened that Emil and I were working on the same function, but the overlap was minor. I have implemented the compression functionality I had in mind (1fb77f2) and added it to the function gen_rational that Emil implemented (8bff5c4). Now the powers of the input matrix that are used in both numerator and denominator are computed only once.

In principle, the function compress_graph_redundant! could be called directly in merge_graphs, at least for nodes of type :mult and :ldiv (this is the default behaviour, but can be overwritten by issuing the kwarg compress_lincomb=true).

[jarlebring]

The compress_graph_redundant! looks good. 👍 I'm still not sure how this fits with the original question. Adding a feature at a fundamental level (eval_graph or Compgraph) should be thought about carefully if it is really needed. It may require adding support elsewhere, e.g., topological sorting, saving functions, code generation etc, which is maybe not the worth the effort. Code generation is already quite complicated.

[mfasi]

I implemented the solution that adds the input kwarg (which defaults to :A) to eval_graph in a separate branch (eval_graph_input_kwarg). If the solution sounds reasonable, we can move the commit to the main branch, otherwise we should probably just close this issue.

[jarlebring]

It looks okay. Could you add test?

Also, for consistency eval_jac should probably also have input for consistency.

[jarlebring]

Maybe you want to turn your branch into a PR if you want to have further discussion?