Skip to content

Fix default constructor of QubitRegister #94

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: development
Choose a base branch
from
Open

Fix default constructor of QubitRegister #94

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/qureg_init.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,9 @@ QubitRegister<Type>::QubitRegister()

fusion = false;

Resize(1UL);
state_storage[0] = {1., 0.};
Resize(2UL);
Copy link
Contributor

Choose a reason for hiding this comment

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

QubitRegister::Resize() will be deprecated in the next release, but for the moment this is ok.

Always keep in mind that the argument of Resize corresponds to the new number of global amplitudes.
IQS requires at least 2 amplitudes per MPI rank so calling Resize(2) works only for 1 MPI rank.

This points to a consistent use: the argument-less constructor for QubitRegister should be used only to create a 1-qubit state in state |1> and when a single MPI rank is used.

state[0] = {1., 0.};
state[1] = {0., 0.};

if (nprocs > 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The assert will trigger if the argument-less constructor is called with more than 1 MPI ranks.

The current unit tests are not capturing this failure since they are located in one_qubit_register_test.hpp and all its tests are skipped when there is more than 1 MPI rank (see lines 26-27 there).
If you add an identical test "IntializeWithDefault" to state_initialization_test.hpp you will see it fails when 2 or more MPI ranks are used.

fprintf(stderr,
Expand All @@ -57,7 +58,7 @@ void QubitRegister<Type>::Resize(std::size_t new_num_amplitudes)
log2_nprocs = iqs::ilog2(nprocs);

// FIXME GG: I believe this limits the use of "resize" to adding a single qubit
if(GlobalSize()) assert(GlobalSize() * 2UL == new_num_amplitudes);
// if(GlobalSize()) assert(GlobalSize() * 2UL == new_num_amplitudes);
Copy link
Contributor

Choose a reason for hiding this comment

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

Resize() will be deprecated in the next release in favor of a more descriptive method that clarifies how the state is expanded or reduced in size.

num_qubits = iqs::ilog2(new_num_amplitudes);

local_size_ = UL(1L << UL(num_qubits - log2_nprocs));
Expand Down
14 changes: 14 additions & 0 deletions unit_test/include/one_qubit_register_test.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,20 @@ class OneQubitRegisterTest : public ::testing::Test
//////////////////////////////////////////////////////////////////////////////
// Test macros:

TEST_F(OneQubitRegisterTest, InitializeWithDefault)
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned in another comment, this unit test is run only when there are at most 1 MPI ranks.

{
ComplexDP amplitude;

iqs::QubitRegister<ComplexDP> psi_0;
// |psi_0> = |0>
amplitude = psi_0.GetGlobalAmplitude(0);
ASSERT_DOUBLE_EQ(amplitude.real(), 1.);
ASSERT_DOUBLE_EQ(amplitude.imag(), 0.);
amplitude = psi_0.GetGlobalAmplitude(1);
ASSERT_DOUBLE_EQ(amplitude.real(), 0.);
ASSERT_DOUBLE_EQ(amplitude.imag(), 0.);
}

TEST_F(OneQubitRegisterTest, InitializeInComputationalBasis)
{
ComplexDP amplitude;
Expand Down