Fix default constructor of QubitRegister #94
Conversation
| state_storage[0] = {1., 0.}; | ||
| Resize(2UL); | ||
| state[0] = {1., 0.}; | ||
| state[1] = {0., 0.}; | ||
|
|
||
| if (nprocs > 1) { |
There was a problem hiding this comment.
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); | |||
There was a problem hiding this comment.
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); | |||
There was a problem hiding this comment.
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) | |||
There was a problem hiding this comment.
As mentioned in another comment, this unit test is run only when there are at most 1 MPI ranks.
|
Thank you for your comments. Please feel free to incorporate any changes as seen fit. |
As I explained in Issue #91, the default constructor of
QubitRegisterwas not working properly. Based on what the default constructor needs, I made the constructor allocate a length-2statefor a single qubit and initialize the two elements to 1.0 and 0.0. The lineif(GlobalSize()) assert(GlobalSize() * 2UL == new_num_amplitudes);fails becauseglobal_size_is not initialized and can be an arbitrarily large number when using the default constructor. And since this constrainsResizeto 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.hppto test the behavior of the default constructor.