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.
@Christine Simpson, 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.
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!
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.
@Brian O'shea 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.
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?
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?
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...
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.
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 @Greg Bryan 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 @Nathan Goldbaum 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?
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!)