Improving MustRefineParticle flagging : Fixes Issue #37

#369 Merged at 2c238cb
Repository
aemerick
Branch
week-of-code
Repository
enzo
Branch
week-of-code
Author
  1. Andrew Emerick
Reviewers
Description

Improving must refine particles to move conditionals on what is a must refine particle to the Grid C routine, allowing for more complex conditionals without having to modify Fortran routine or pass new information to Fortran routine.

Comments (14)

  1. Greg Bryan

    Except for the treatment of the MustRefineParticlesMinimumMass parameter, this looks good to me (but a test would be useful).

    1. Andrew Emerick author

      For clarification given your comments below, the user would then set MustRefineParticlesMinimumMass to some mass in MassUnits?

      I guess I'm still stuck on how this makes sense then if it is compared to ParticleMass, which is in code mass units and depends on refinement level. It looks like the code reads in MustRefineParticlesMinimumMass and converts it to code mass units on the level MustRefineParticlesRefineToLevel. So to me that looks like the code in Grid_DepositMustRefineParticles only makes sense from a units stand point if the comparison is made when the particle is in fact already sitting on MustRefineParticlesRefineToLevel, in which case it doesn't actually do anything. This is also because if it is on a refinement level less than MustRefineParticlesRefineToLevel, it will always have a ParticleMass greater than MustRefineParticlesMinimumMass.

      Does my understanding of this make sense?

  2. Greg Bryan

    Here is my (possibly flawed) understanding. In `ReadParameterFile.C, there is the following code:

      /* convert MustRefineParticlesMinimumMass from a mass into a density,
         ASSUMING CUBE simulation space */
        MustRefineParticlesMinimumMass /= POW(1/(float(MetaData.TopGridDims[0])
                                           *POW(float(RefineBy), float(MustRefineParticlesRefineToLevel))),3);
    

    and later:

        /* Change input physical parameters into code units */
        MustRefineParticlesMinimumMass /= MassUnits;
    

    So I think this takes a real mass in MassUnits and converts it to code density units (the order in conceptually backwards, but that doesn't affect the result).

      1. Andrew Emerick author

        Ya I agree with how this is handled in Read/Write, but there still may be an inconsistency. Or I'm misunderstanding.

  3. Greg Bryan

    @jwise77 Do you know anything about this? I think that this parameter is not actually used for the standard case of MustRefineParticles that Christine Simpson put in. In fact, the code makes that clear:

      /* Special case on level == MustRefineParticlesRefineToLevel when we
         restrict the additional AMR to regions with must-refine
         particles, and don't use the particle mass field. */
    
      int NumberOfFlaggedCells = 0;
      if (!(ProblemType == 30 && MustRefineParticlesCreateParticles == 3 &&
            level == MustRefineParticlesRefineToLevel)) {
        for (i = 0; i < size; i++)
          if (FlaggingField[i]) {
            ParticleMassFlaggingField[i] = MustRefineMass;
            NumberOfFlaggedCells++;
          }
      }
    

    So I think this is why I am confused by this parameter -- I've never used it. The default value of this parameter is zero, and so all particles are larger than this. I do agree there is a potential inconsistency here, but I'm not sure what the goal was here. This minimum mass code appears to have been written by Elizabeth Harper-Clark. I suspect it is no longer in use and can actually be removed (although not 100% sure).

    1. John Wise

      I've actually never used it before, and you're right that it's not used in the Christine's work either.

  4. Greg Bryan

    But if we remove it as a settable parameter, then it should probably be pulled out everywhere. Alternately, you can leave it in, in which case I would leave the parameter as is.

  5. chummels

    So I tweaked the AMRCosmology test to incorporate MRPs to test this PR. I essentially just identify the densest location in the after the first output at redshift 10 or something, then tag the 100 particles closest to this point as MRPs. Then I rerun the simulation using those particles as MRPs forcing refinement on MRPs. When I do this for the tip of enzo, it works and forces refinement on this halo as expected. When I do this for the tip of your PR, it doesn't force refinement due to the presence of MRPs. I can help you look for where things are going wrong, but just wanted to let you know.

  6. chummels

    OK, so with @aemerick 's change, this appears to reproduce exactly the behavior of the previous MRPs. As a demo, here are two movies (left side = tip of enzo, right size = this PR):

    Count of MRPs in projection (max in projection):

    emerick.gif

    Grid Level in projection (max in projection):

    emerick2.gif

    Interestingly, I see a performance boost of about 10% the runtime for the run when include this PR, although it was on a laptop with other stuff going on. So I'd say it's about the same. Thanks for the new functionality!

  7. Andrew Emerick author

    I've reverted changes that removed MustRefineParticlesMinimumMass from being an external parameter. This is now back as it was.

    I still think there is a bit of an issue in what this parameter actually does, and its not clear to me what it was intended to do. This is worth a bit more discussion before merging this PR. (see my reply to Greg at the very top of the page)