fix issue of reading params

[typerSniper]

It is my attempt to fix the issue #124, which was possibly introduced by PR #123. @zikun-li, can you please check?

In an unrelated change, I also edited a line in substitution.cpp that was tripping earlier.

[xumingkuan]

This may be a quick way to fix it but we need to make sure that not assigning the parameter values won't break other things.

Why do we have the context object not containing the input parameter values? How did you load the QASM file (like, using the function from Graph or from CircuitSeq)?

[typerSniper]

Great questions, I couldn't figure out how the loading works. But, I saw this line in the prior code, which also assigns to the map, only if the context object contains the input parameter values. Only the recent PR always assigns, which maybe a good thing, but I couldn't figure it out.

Edit: what I can't figure out is how the index of a node (in CircuitSeq) is related to the indices of context->input_parameters

[zikun-li]

The node->index is a index into context->input_parameters (i.e. you can find the value of the parameter represented by node by context->input_parameters[node->index]. Is this correct? @xumingkuan

[xumingkuan]

The node->index is a index into context->input_parameters (i.e. you can find the value of the parameter represented by node by context->input_parameters[node->index]. Is this correct? @xumingkuan

Note that node->type == CircuitWire::input_param, so node->index is essentially the input parameter index. If the circuit is loaded using the function in CircuitSeq with the same Context object, the Context will store the input parameter value there.

[typerSniper]

I don't think the problem comes from the qasm_parser. I think the bug is in CircuitSeq::read_json, which does not push parameters to context->input_parameters. When a circuit_seq generated by CircuitSeq::read_json is converted to a graph, the bug above comes, because the context has not been updated.

[zikun-li]

I see. The problem is that CircuitSeq can have parameters that are symbolic (i.e. without a constant value), but in the Graph::Graph(Context *ctx, const CircuitSeq *seq) API, we assumes that all parameters have a constant value. This should be fixed indeed.

But why you are constructing a Graph from a CircuitSeq generated by CircuitSeq::read_json?

[typerSniper]

But why you are constructing a Graph from a CircuitSeq generated by CircuitSeq::read_json?

@zikun-li, it happens every time we use equivalence classes. We read the file, parse equivalence classes as circuit sequences (using read json), and then generate graph transformations by converting the sequences to graphs.

update:: is it easy to check when a parameter is symbolic and handle it appropriately in the API?

[typerSniper]

Could we parse symbolic parameters as internal_params? would that work

[xumingkuan]

Oh right, so Graph must be able to support symbolic parameters.

For CircuitSeq, I assumed the Context to have either all parameters symbolic or all parameters concrete. I don't know what assumptions are made about symbolic parameters in Graph.

[zikun-li]

Actually in Graph we allow both symbolic and constant parameters. The only difference between then is that symbolic parameters don't have entries in Graph::constant_param_values, and we don't have support to convert Graph with symbolic parameters to qasm.

[xumingkuan]

I see. Maybe let's merge this PR for now and I'll make a better API to show if a parameter in CircuitSeq is symbolic.

[zikun-li]

Btw, I don't think you need to convert CircuitSeq to Graph to construct transformations, because the transformations are expressed with the class GraphXfer. @typerSniper