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

Refactor JSON output to prevent concurrency bugs #66

Merged
merged 3 commits into from
Sep 6, 2018

Conversation

femtobit
Copy link
Collaborator

@femtobit femtobit commented Sep 5, 2018

The JSON output bug fixed by #64 was caused because converting an ObsManager to JSON performs an MPI reduction and thus has to be called from all processes, while in the first version of my code it was only called by rank 0.

This PR adds a few changes to the output code to make errors like this less likely.

…PI workers

The imaginary time propagation currently is not parallelized, so using multiple
workers in not sensible yet.
@gcarleo
Copy link
Member

gcarleo commented Sep 5, 2018

@femtobit , thanks, I will look into this. A further issue we just identified is that now we are appending to the wave function file, instead of rewriting it from scratch, as we were doing before. The expected behaviour is to save the wave-function for backup purposes, so appending to the old file leads to undefined behaviour when loading the wave-function in further calculations.

@femtobit
Copy link
Collaborator Author

femtobit commented Sep 5, 2018

A further issue we just identified is that now we are appending to the wave function file, instead of rewriting it from scratch, as we were doing before.

Ah, I see. I changed the code to rewrite the wavefunction every time.

Maybe we should make this configurable, as it could be useful for tests with small systems to have the complete state information saved?

@gcarleo
Copy link
Member

gcarleo commented Sep 6, 2018

@kchoo1118 what's the current status with the wave function log file then? You mentioned that this issue might be already solved in more recent commits?

@kchoo1118
Copy link
Collaborator

yea i just modified it in my local branch so that the wavefunction is not constantly appending

@gcarleo
Copy link
Member

gcarleo commented Sep 6, 2018

OK then I will merge this PR, and maybe we can do another PR for the wavefuncion appending issue @kchoo1118 :)

Copy link
Member

@gcarleo gcarleo left a comment

Choose a reason for hiding this comment

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

Great, it looks MPI safer now, thanks!

@gcarleo gcarleo merged commit 4e805a8 into netket:master Sep 6, 2018
@femtobit femtobit deleted the output branch September 6, 2018 16:23
@NnktYoshioka NnktYoshioka mentioned this pull request Nov 25, 2018
gcarleo added a commit to gcarleo/netket that referenced this pull request Feb 14, 2019
Refactor JSON output to prevent concurrency bugs
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.

None yet

3 participants