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

make_resume_file() incompatible with MPI #101

Merged

Conversation

AdamOrmondroyd
Copy link
Contributor

pypolychord/polychord.py::make_resume_file() does not work with more than one MPI process.

It looks like the send and receive buffers were mixed up, though I admit I have absolutely no idea why recvbuf.dtype was set to int.

I have visually checked that the live points written to the resume file by make_resume_file() are the same numbers as the provided PolyChordSettings.cube_samples.

(I should probably have suspected something would go wrong after not personally pushing my changes in #92 very hard, and here we are)

@AdamOrmondroyd AdamOrmondroyd changed the title no idea why recvbuf.dtype was int, and is only not empty for rank 0 make_resume_file() incompatible with MPI Oct 5, 2023
@williamjameshandley
Copy link
Member

Good spot! How easy is it to write a test for this?

@AdamOrmondroyd
Copy link
Contributor Author

It'll be faster to write than decide whether or not it's easy

@AdamOrmondroyd
Copy link
Contributor Author

I think this is actually worth some thought.

As things stand, the simplest thing to do would be to create another run_pypolychord.py file which provides some cube samples (not really important what they are).

The natural and easy place for this would be in the Python_Functions folder (which seriously needs tidying up)

However, if we're going to make a test for this, I see no reason why every single other option doesn't also deserve a test. I envisage something like:

@pytest.mark.parameterize("settings", [some, variants, of, settings])
def test_run(settings):
    ...
    output = pypolychord.run_polychord(likelihood, nDims, nDerived, settings, prior, dumper)

However, this would all need to change to plain old kwargs instead of PolyChordSettings if the neglected #90 ever gets merged.

@AdamOrmondroyd AdamOrmondroyd mentioned this pull request Oct 6, 2023
@AdamOrmondroyd
Copy link
Contributor Author

I've added the optional dependency cube_samples which installs fortranformat which is needed for cube_samples to work properly

@AdamOrmondroyd
Copy link
Contributor Author

Patch version bump?

@AdamOrmondroyd AdamOrmondroyd merged commit bd24bc7 into PolyChord:master Jan 9, 2024
7 checks passed
@AdamOrmondroyd AdamOrmondroyd deleted the make_resume_cube_samples branch January 9, 2024 13:57
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