Zcurve mpi fix #128
Zcurve mpi fix #128
Conversation
This fixes a regresseion where the initial MPI decomposition and number per mpi domain is called twice, which wipes out the load balanced decomposition. This is a prelimary commit. Requires further testing to be certain all changes from another fork are present
There was a problem hiding this comment.
At first I was a bit taken aback by the big line count of the diff, but after some inspection it turns out most of it is removal of commented code. I think we can take that, but I still sprinkled some questions here and there about some changes that seem inoffensive, but for which I'd like some confirmation.
I also haven't really tested these changes myself. I think Chris said he was going to play with them, so I'll leave that testing to him.
| @@ -324,6 +324,11 @@ typedef void (*ExtraPropFunc)(double, double, double&); | |||
| #define Grav_in_kpc_kms_solarmasses 4.3022682e-6 | |||
| //@} | |||
|
|
|||
| ///\defgroup Z-curve Mesh constants | |||
| //@{ | |||
| #define MESH_MINCELLSPERDIM 4 | |||
There was a problem hiding this comment.
is this change in value (8 -> 4) intentional and part of the fix?
There was a problem hiding this comment.
I found for very small sims you didn't need to have a mesh size of 8^3 so 4^3 seemed like a good min size. This change is itself unrelated to the bug fix but does at least also remove a constant that I had hardcoded with a defined constant.
| if (ThisTask==0) { | ||
| /* |
There was a problem hiding this comment.
this if (and the correspondig closing }) are all what's left in this function; do you want to remove them as well?
There was a problem hiding this comment.
Sure, I can remove this as well.
| MPIInitialDomainDecomposition(opt); | ||
| MPIDomainDecompositionGadget(opt); | ||
| } | ||
| Int_t i,k,n; |
There was a problem hiding this comment.
Changes from this line onwards in this function seem like an unrelated change?
There was a problem hiding this comment.
I did have some small changes cleaning up the gadget code. Not related to the fix and so can remove this if desired so it is a separate PR.
| //PredType HDFREALTYPE(PredType::NATIVE_FLOAT); | ||
| int ireaderror = 0; | ||
|
|
||
| LOG(debug) <<" running domain extent"; |
There was a problem hiding this comment.
All changes in this file look only like formatting changes (removing commented code, re-indenting after removing a {/} block), is that correct? As such, it seems like an unrelated change to this MR. Just want to understand if there's something I'm missing.
There was a problem hiding this comment.
Yes, this is correct.
|
|
||
| Int_t i,j,k; | ||
| unsigned long long n,nchunk; | ||
| char buf[2000]; | ||
| MPI_Status status; |
There was a problem hiding this comment.
Again, from this line onwards in this function it looks like all changes are unrelated to the mesh/mpi decomposition fixes, is that correct?
There was a problem hiding this comment.
Yes, this is not itself related to the mesh, just a code clean-up. Again, happy to revert this and leave the PR to just those related to the mesh.
| // is set such that the cells per dim grows as (log(N)/log(2))^(y). Currently y=1 | ||
| // unless the numcellsperdim has been set, in which case that is used. | ||
| if (opt.numcellsperdim == 0) { | ||
| unsigned int NProcfac = std::ceil(log(static_cast<double>(NProcs))/log(2.0)); |
There was a problem hiding this comment.
| unsigned int NProcfac = std::ceil(log(static_cast<double>(NProcs))/log(2.0)); | |
| unsigned int NProcfac = std::ceil(std::log2(static_cast<double>(NProcs))); |
the comment would also need adjusting
There was a problem hiding this comment.
Ah, yes I'll update the comment too.
| @@ -1145,6 +1145,8 @@ void GetParamFile(Options &opt) | |||
| opt.impiusemesh = (atoi(vbuff)>0); | |||
| else if (strcmp(tbuff, "MPI_zcurve_mesh_decomposition_min_num_cells_per_dim")==0) | |||
| opt.minnumcellperdim = atoi(vbuff); | |||
| else if (strcmp(tbuff, "MPI_zcurve_mesh_decomposition_num_cells_per_dim")==0) | |||
There was a problem hiding this comment.
if this is a new option it would need documentation (don't remember where that lives). Also, is it being already dumped when writing a configuration file from the Options?
There was a problem hiding this comment.
This is the portion that I need to check, hence it still being in draft. The documentation would also need a change.
|
FWIW we don't have big problems in large volumes. (say 1Gpc with 2x2000^3 particles) |
Fixes an issue with redoing the mpi decomposition when running with a baryon search. Tested with flamingo input.
Might need to test the configuration file produced to see if appropriate entry added if configuration file has specific parameter set.