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

sanity check all pickled non-interacting Hamiltonians in h0 directory #52

Merged
merged 37 commits into from
Apr 8, 2024

Conversation

JohanSchott
Copy link
Owner

  • Move sanity checking code of pickled Hamiltonian to a unit-test
  • Test all pickled Hamiltonians in h0 directory
  • Add sanity checks

impurityModel/ed/get_spectra.py Outdated Show resolved Hide resolved
impurityModel/test/test_h0.py Outdated Show resolved Hide resolved
assert len(process) == 2 # two events in non-interacting Hamiltonian
assert process[1][1] == "a" # First event is annihilation
assert process[0][1] == "c" # Second event is creation
for event in process[::-1]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

no need to check events in any particular order.

Suggested change
for event in process[::-1]:
for event in process:

Copy link
Owner Author

Choose a reason for hiding this comment

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

The order does not matter in this for-loop. But when a process is used for real, i.e. applied on a product state, the order matters.

The order is chosen such that $c_i^\dagger c_j \ket{\Psi}$ is implemented as process ((i, 'c'), (j, 'a')) operating on the state $\ket{\Psi}$ (with 'c' denoting creation and 'a' annihilation). So the right-most "event" is first applied, and finally the left-most "event", just like the math expression.
Relevant documentation e.g. https://github.com/JohanSchott/impurityModel/blob/master/impurityModel/ed/finite.py#L1320
https://github.com/JohanSchott/impurityModel/blob/master/impurityModel/ed/finite.py#L1395

And in the same way is the Coulomb interaction process between two electrons:
$c_i^\dagger c_j^\dagger c_k c_l$ implemented as ((i, 'c'), (j, 'c'), (k, 'a'), (l, 'a'))
https://github.com/JohanSchott/impurityModel/blob/master/impurityModel/ed/finite.py#L1322

Because of this I think it might be good to consistently loop over events "from right to left" in this repo.

JohanSchott and others added 2 commits March 26, 2024 21:00
Co-authored-by: kalvdans <github@kalvdans.no-ip.org>
Co-authored-by: kalvdans <github@kalvdans.no-ip.org>
@JohanSchott
Copy link
Owner Author

Thanks for your review @kalvdans !

I struggled to understand why the unit-tests failed in this PR.
At first I also thought I could not reproduce the error locally, so I'm sorry about the many git pushes.
But I can reproduce the problem locally, and I have figured out what the problem was.

MPI can't be run both in a parent process and a child process.
In master no unit-test imports MPI stuff, except in a subprocess in impurityModel/test/test_comparison_with_reference.py.
But in this PR add impurityModel/test/test_h0.py which imports MPI stuff with the line from impurityModel.ed.get_spectra import read_pickled_file. So when run pytest in this PR both the parent and a child process uses MPI stuff, which is not ok.

I the last commits I solve this issue by ignoring impurityModel/test/test_comparison_with_reference.py in pytest.ini and running pytest twice:

  • pytest impurityModel/test/test_comparison_with_reference.py, which uses MPI only in the child process (and not in parent process).
  • pytest, which uses MPI only in the parent process (no subprocess even present in these tests).

Small reproducer of the problem:
python -c "from mpi4py import MPI; import subprocess; output = subprocess.run(args=['mpiexec -n 2 echo hello world'], shell=True, check=False, capture_output=True); print(output)"

But without from mpi4py import MPI it works fine, i.e.:
python -c "import subprocess; output = subprocess.run(args=['mpiexec -n 2 echo hello world'], shell=True, check=False, capture_output=True); print(output)"

@kalvdans
Copy link
Collaborator

kalvdans commented Apr 2, 2024

It seems MPI is automatically initialized when you import mpi4py.MPI (imports should never have side-effects in my opinion).

So just refrain from importing MPI until during runtime when you want to start parallellizing stuff. That also mean that you can't read the constant mpi4py.MPI.COMM_WORLD beforehand :(. And don't test MPI stuff using pytest since you can't restart MPI once you have stopped it...

$ python -c "from mpi4py import MPI; MPI.Finalize(); MPI.Init()"
*** The MPI_Init() function was called after MPI_FINALIZE was invoked.
*** This is disallowed by the MPI standard.
*** Your MPI job will now abort.
[brahe:467487] Local abort after MPI_FINALIZE started completed successfully, but am not able to aggregate error messages, and not able to guarantee that all other processes were killed!

@JohanSchott
Copy link
Owner Author

It would be good to test some MPI stuff but not sure how to do that.

I guess one way to test MPI stuff is something like:

def test_MPI_stuff_of_function_XXX():
    tot = []
    for ranks in [1,2,4,8]:
       subproces.run(f"mpiexec -n {ranks} python a_script_that_calls_function_XXX_and_save_its_output.py output_{ranks}", shell=True)
        tot.append(read_file(f"output_{ranks}"))
    compare_data(tot)

where read_file reads e.g. a pickle or hdf5 file.
But it's not so elegant. And one python script for each unit-test...

@kalvdans
Copy link
Collaborator

kalvdans commented Apr 2, 2024

I solve this issue by [...] running pytest twice

Seems it is the same solution pytest-mpi have chosen, and have two decorators to mark up which test should be run in which invocation.

Copy link
Collaborator

@kalvdans kalvdans left a comment

Choose a reason for hiding this comment

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

Small items left, LGTM!

impurityModel/test/test_comparison_with_reference.py Outdated Show resolved Hide resolved
x_ref = ref_file_handle[key][()]
abs_diff = np.abs(x - x_ref)
i = np.argmax(abs_diff)
print("Max abs diff:", np.ravel(abs_diff)[i])
Copy link
Collaborator

Choose a reason for hiding this comment

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

assert_allclose will already print the maximum absolute diff.

Copy link
Owner Author

Choose a reason for hiding this comment

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

true. I'll remove the print statement

pytest.ini Outdated Show resolved Hide resolved
JohanSchott and others added 3 commits April 8, 2024 22:14
Co-authored-by: kalvdans <github@kalvdans.no-ip.org>
Co-authored-by: kalvdans <github@kalvdans.no-ip.org>
@JohanSchott JohanSchott merged commit 6d2e192 into master Apr 8, 2024
1 check passed
@JohanSchott JohanSchott deleted the sanity_check_all_pickled_hamiltonians branch April 20, 2024 20:03
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