Skip to content

Support multiple input/output states.#270

Open
s-mandra wants to merge 7 commits intomasterfrom
multiple_input_output_states
Open

Support multiple input/output states.#270
s-mandra wants to merge 7 commits intomasterfrom
multiple_input_output_states

Conversation

@s-mandra
Copy link
Owner

@s-mandra s-mandra commented Feb 4, 2020

Improve code to handle multiple initial/final states.

@s-mandra s-mandra self-assigned this Feb 4, 2020
@s-mandra s-mandra marked this pull request as ready for review February 5, 2020 16:31
Copy link
Contributor

@95-martin-orion 95-martin-orion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First pass.

@@ -58,12 +62,33 @@ std::vector<std::vector<std::size_t>> read_grid_layout_from_stream(
* be populated by this method.
* @param output_states vector of output states for the given contraction
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update comment (output_states is no longer a parameter)

/**
* Given a grid of 3D tensors, rename indexes of the terminal tensors to
* reflect terminal qubits.
* @param QflexFinalQubits representing the final qubits.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For specifying parameters in function comments, please use the format:

* @param <parameter_name> <parameter_description>

Also, it looks like some parameters are missing from this comment.


/**
* Apply the final delta-tensors to the 3D grid of tensors.
* @param QflexGrid representing the circuit layout.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See previous comment.

*/
std::vector<std::pair<std::string, std::complex<double>>> EvaluateCircuit(
QflexInput* input);
std::vector<std::tuple<std::string, std::string, std::complex<double>>>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this change, the @return spec is no longer accurate - please update.

qflex::global::track_memory_seconds, R"(].
--initial-conf=<initial_conf> Initial configuration.
--final-conf=<final_conf> Final configuration.
--initial-conf=<initial_conf> Initial configuration [default: 00...00].
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This help message should indicate that multiple input/output strings are supported, and (briefly) describe how qFlex handles them.

Copy link
Contributor

@95-martin-orion 95-martin-orion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I still need to review evaluate_circuit.cpp, but this should cover the rest.

}
}

// Delete duplicate initial/final configuration
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd actually suggest that we return an error if duplicates are detected. The reasoning is this: if a user manually specifies two input states, they probably meant to provide two different input states. Since a simulation run can take a long time, we should fail early so the user can correct their mistake.

amplitudes.push_back({is, EvaluateCircuit(&input)});
}
}
// Remove duplicate initial/final states
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above - duplicate states should produce an error.

@@ -320,18 +318,10 @@ void circuit_data_to_tensor_network(
}

// Check for the length of initial_conf and final_conf.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update comment.


/**
* Return gate as an array.
* @param name of the gate.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@param formatting.


/**
* Read circuit from stream and fill in a 2D grid of vectors of tensors.
* @param qflex::QflexCircuit containing circuit information.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update function comment to reflect parameter changes. (Also looks like the first @param is missing its parameter name.)

* work.
*/
void flatten_grid_of_tensors(
std::vector<std::vector<Tensor>> flatten_grid_of_tensors(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update function comment.

@@ -9,15 +9,18 @@ namespace {
class GetOutputStatesTest : public testing::Test {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

File comment: could you add tests for multiple-(input/output) and the new functions (apply_terminal_cuts and apply_delta_output)?

tensor_grid_3D, &tensor_grid_2D, &scratch);

// Reorder the 2D grid
reorder_grid_of_tensors(&tensor_grid_2D, final_qubits.qubits,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would a separate test for reorder_grid_of_tensors make sense, or does this provide enough coverage as-is?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants