#309 Merged at ab70454
  1. Christine Simpson

This isn't perfect, but in the interests of getting the ball rolling on this, here is the MRP pull request. This includes:

  • multiple new methods for initializing MRPs in cosmological nested simulations
  • a TestOrbit type test problem
  • modifications to subgrid creations that takes into account the subgrid ratio. The user can now specify a maximum subgrid ratio that is acceptable. This change may cause problems with the test suite, however I found it necessary for running ellipsoidaly masked MUSIC ICs.
  • Documentation on MRPs

Comments (30)

  1. John Wise

    This is great, Christine. Thanks very much for creating this PR. I will review and test it out. @chummels can you look at it, too? Thanks!

  2. John Wise

    First, could you resolve the conflicts in Grid_SetFlaggingField.C and Make.config.objects?

    1. Christine Simpson author

      Okay, I think I've accomplished this (I was a bit confused on how to do this because it was conflicting with empty lines).

  3. Britton Smith

    I think you need to remove the line in Make.config.objects with "Grid_FlagCellsToBeRefinedByMustRefineParticles.o" on it. Otherwise, Make seems to have problems.

  4. Britton Smith

    @cms2166, thanks for fixing all my previous comments. I have tested this out and it works quite well for me. I'll now give the PR a final read through for typos and other such issues. I'll leave another comments when I'm done.

    1. Britton Smith

      I've looked over everything and it all looks good to me. I've left a few comments below that are mostly just typos and deleting commented out lines. As I said earlier, I've tested this out in some cosmology runs and it works great. The added test problem also works fine for me. This is an awesome feature!

      1. Christine Simpson author

        Thanks for your help on this, Britton. I've gone through all your comments and I think I addressed everything. A comment on the bisection search: I copied this from another part of the code (maybe in the halo finding routines?), and it should make this piece of code a bit faster (although it is a bit of a silly optimization in this part of the code, tbh). However, it is important that the list of ids be sorted in the input file for this to work. I have added a note in the parameter documentation about this.

        1. Britton Smith

          Ok, that's fine. I just couldn't quite tell what it was doing. Thanks for adding an extra note!

  5. Brian OShea

    This PR (at changeset dc9cd2d39de8) actually breaks the push suite - there are 119 test failures out of 1779. See http://galactica.pa.msu.edu/~bwoshea/misc/PR309_test_results.txt for the entire output of the test suite using the "push" suite. @jwise77 , @gbryan , @cms2166 , @chummels - do you have any idea why this set of tests might be failing? I've run several PRs with the same compilation/compiler settings and versus the same gold standard, and this is the only one that fails.

    1. John Wise

      Maybe it's caused by how proto-subgrids are created? There are some changes that restrict the ratio between the min/max edge lengths. It seems that most (if not all?) of the failed tests use AMR.

    2. John Wise

      It looks like ShockPool2D is failing, which doesn't use AMR. Maybe this is a good test case to find the changeset that causes the differences.

    3. John Wise

      Here is a list of the failed tests, and they all use AMR.

      AMRShockPool2D, FreeExpansionAMR, NohProblem2DAMR, ShockPool2D, MHD2DRotorTest, SedovBlast-MHD-2D-Fryxell, SedovBlast-MHD-2D-Gardiner, PhotonTestAMR

      I think the differences are being caused by the changes in the proto-subgrid routines.

      1. Brian OShea

        Oh, hmm, I guess I thought that the MHD rotor test and the Sedov test weren't AMR. Nice catch!

    4. John Wise

      @bwoshea Is it possible to run the push suite after backing out the changes in IdentifyNewSubgridsBySignature.C? Then we'll know for sure whether these changes are cause. However, I think these changes are necessary to include in the PR.

        1. Brian OShea

          followup on this: reverting IdentifyNewSubgridsBySignature.C to the pre-MRP version completely fixes the problem, so it's definitely that.

    5. Christine Simpson author

      I need to read through all the comments, but I think this is likely what I warned you about with the new limit on the subgrid ratio. Can you try changing the default value for CriticalGridRatio to a large value? like 100? What fraction of tests fail then?

      1. Brian OShea

        When I change CriticalGridRatio to a default value of 100, the exact same tests fail. John has argued that we definitely want to keep the changes in IdentifyNewSubgridsBySignature.C, though - it may be that this simply changes the gold standard. What exactly does the changed code do?

        1. Greg Bryan

          It does two things: (1) splits the grid in half along the longest dimension if the ratio of largest to smallest length is above some critical value (this is done before checking the signatures), and (2) only splits at an inflection point in the signature if the resulting grids have dimensions that are both larger than MinimumSubgridEdge. We did (1) because we were finding lots of long thin grids which were not split along the logical dimension because they had no inflection point in the second derivative signature. This seems like a design flaw in the original implementation which was not noticed because it doesn't occur very often (but does in this case). The second change (2) was implemented in part because it helped to make sure that we were not generating long thin grids because of pathological signatures, and also because it really should be there -- the original parameter name seems to imply a promise ("MinimumSubgridEdge") that was not delivered.

          In my opinion, these changes both make sense and should be incorporated in Enzo generally. I expect that they will break the gold standard for AMR runs because they will generally result in different grid partitions (more sensible ones I would hope). Given that, it might make sense to move the changes here (and in ProtoSubgrid_CriticalAxisCheck.C) into it's own PR...

          1. Nathan Goldbaum

            What about including these AMR changes in a separate pull request so they can be reviewed independently of the must-refine particle work?

            I haven't looked at the changes closely, but if it does what Christine just described then it does indeed sound like a worthwhile improvement to Enzo's AMR.

          2. Christine Simpson author

            Greg is exactly correct. The adjustments to to MinimumSubgridEdge I think is why just changing the CriticalGridRatio doesn't necessarily help. I haven't looked at the outputs of the test suite, but my hope would be that these failures are just due to a different grid partitions and that the new partitions are better. If this is the case, I would argue that the Gold Standard should change.

            This pull request should not be accepted without these changes to the grid partition incorporated first. That is to say the code will run, but an uncareful user will likely not notice the poor grid partitioning. In tests of the 1e11 Msun quiescent AGORA halo with elliptical masking, I found at high z it was not uncommon to have grids that were 8 cells wide in one dimension (3 ghost cells + 2 real cells + 3 more ghost cells) and had a grid ratio > 20. It ran with these slab-type grids, but grids that are only two cells wide in one direction and have a ratio > 20 are not good. If I hadn't been paying close attention to the grid partitioning, I may have not noticed. I think it has something to do with the nature of signatures that result from a curved surface on the cartesian mesh.

            Could either John or Brian email me a list of failed tests so I can take a look at a couple? I'm not sure how separating this change into a separate pull request will make the workflow on this issue more efficient.

            1. Brian OShea

              Christine: the failed tests and accompanying info are at http://galactica.pa.msu.edu/~bwoshea/misc/PR309_test_results.txt . I don't think that you'll see the same sorts of behavior you described in your comment, since the test problems typically have simple geometries. That said, I think what you and @gbryan are saying is sensible - the change to AMR behavior is something that is desirable, and furthermore is necessary for this PR to function correctly. As a result, I don't think that we should split this off into a new PR as @ngoldbaum suggested. Perhaps separating the PRs makes more sense from the historical perspective, but in an operational sense it doesn't.

              I think we should accept the PR as-is (modulo a couple more approvals from reviewers), and just let people know via the enzo-dev mailing list that there will be some change to the default AMR behavior. It will break the gold standard, but that's really not a problem - since I'm doing all the testing on my local machine, it's a simple matter to regenerate the gold standard.

              Does this seem like a sensible way forward to everybody?

              1. Christine Simpson author

                Sounds good to me. Just glancing at the list of failures, it seems like most of the problems have some kind of spherical symmetry?

                1. Brian OShea

                  You're right. I think that the only exception is ShockPool2D, but that has a shock interacting with the domain boundaries in a way that probably is sensitive to this particular part of the code (i.e., how grids get created in space). Anyway, I've looked at almost all of the test outputs and they seem reasonable, so I'm ok with approving this as-is. (Modulo a bit of code review by a 3rd person, that is!)

  6. John Wise

    In other news, I just tested these changes on one of my MRP simulations (N-body only), and it worked flawlessly. My executable was made after a merge with the mainline tip.

  7. Brian OShea

    Hi all, it passes the test suite and any remaining comments are cosmetic. In the interest of progress, I think it's time to merge this PR. Thanks to all who helped out on this one!