Skip to content
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.

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

Fix default constructor of QubitRegister #94

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

Conversation

snsunx
Copy link

@snsunx snsunx commented Jun 12, 2023

As I explained in Issue #91, the default constructor of QubitRegister was not working properly. Based on what the default constructor needs, I made the constructor allocate a length-2 state for a single qubit and initialize the two elements to 1.0 and 0.0. The line if(GlobalSize()) assert(GlobalSize() * 2UL == new_num_amplitudes); fails because global_size_ is not initialized and can be an arbitrarily large number when using the default constructor. And since this constrains Resize to only adding a single additional qubit, I think this line can be deleted.

I also wrote a simple test in unit_test/include/one_qubit_register_test.hpp to test the behavior of the default constructor.

state_storage[0] = {1., 0.};
Resize(2UL);
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.

@@ -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.

@@ -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.

@@ -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.

@snsunx
Copy link
Author

snsunx commented Aug 9, 2023

Thank you for your comments. Please feel free to incorporate any changes as seen fit.

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.

2 participants