Update: This has been fully tested. The following tests change due to the change in the position of SetTimeNextTimestep. This change is correct, the prior operator order was incorrect.
Things that were different between the two that needs to be examined and discussed:
1. Does OutputFromEvolveLevel happen before or after the recursive call to EvolveLevel?leave as is
2. Does ComovingExpansion happen immediately after the hydro update, or after Multispecies and StarParticles? Leave as is
3. When does SetTimeNextTimestep get called? Immediately after the first hydro phase, or at the end of the loop?end of the loop
No testing has been done.Fully tested
Here are my two bits (to summarize discussion):
I have no preference on order of OutputFromEvolveLevel/EvolveLevel (except that it should be consistent between RK and non-RK).
There is no good answer to this in the context of operator splitting. This should probably be tested empirically, but in the absence of evidence I don't see an argument to changing.
I definitely think SetTimeNextTimestep should come at the end, both because it is the logical place for it to be and because all of the Grid_ update routines assume that they are at the "old" time (and do things like compute a time-centered expansion factor with a+0.5*dtFixed).
I went through Dedner, and I believe that the SetTimeNextTimestep belongs at the end for that, as well, and was incorrectly placed before. I believe that the current state of this is the correct order of operations, and can be reviewed and tested.
@dcollins4096, it may be useful to activate pipelines for this PR to have the test suite run automatically. If you haven't done it, you'll have to pull in the tip of enzo-dev and merge it, but then that should work. The tests take about 70 minutes right now, so you might not want to activate pipelines until you think this PR is about done, so that you don't run out of build minutes.
70 is a lot, is it straight forward to toggle that on and off? I wind up pushing a lot of little inconsequential changes to this particular fork, but I also issue PRs from it.
@dcollins4096 What is the status of this PR? What do you need from reviewers right now?
It passes the test suite except for a couple of RK2 tests, which is what I expected. I need to examine those tests to make sure that the new results are reasonable. Might take a couple days until I can get to it, though.
I'm ready for reviewers now, or it can wait until I've summarized the new tests.
Roger that. I'll wait until you've summarized the new tests, then round up some reviewers. Thanks!
@dcollins4096 - a quick followup on this, given the phone call we had a couple of days ago: I am going to ask @pgrete to look at the PR and give his opinions.
I do have three followup questions for you and @yipihey :
Which tests were breaking, and did you examine them to see if they were reasonable? (I'm referring back to the note from 2017-08-17 above.)
What physics is in EvolveLevel_RK2 that is NOT in EvolveLevel? In other words, our mission is to completely remove EvolveLevel_RK2 without losing any functionality, so what needs to be copied over or ported to EvolveLevel?
Given the two questions above, what can the rest of the Enzo community do to help facilitate the merger of EvolveLevel and EvolveLevel_RK2, beyond reviewing and testing this PR?
I have not looked at the tests yet so cannot answer question 1.
Question 2: Yes. We are hoping to get rid of EvolveLevelRK2 completely.
This should help keeping any further development functioning for all hydro/MHD methods in sync without having to duplicate code in two separate places.
It’ll make things simpler and more transparent.
Question 3: Anyone running simulations with HydroMethod 3 or 4 who could give this version a try to duplicate their current results could help us give confidence in that the merge is working.
Thanks, @yipihey ! This is very helpful. I looked a bit more at the tests that have errors in the test suite (which is looking for bitwise-identical results, and erroring if not), and they are:
The only one of these that surprises me is FreeExpansionAMR, and the differences in that are relatively minor.
If anybody is using sims with HydroMethod 3 or 4, could you chime in and offer to test things please? Thank you!
I fixed one missing bug, a call to PoissonSolver that I had missed. That has been pushed.
Now, of the six tests that fail, all but two are attributable to the order of SetTimeNextTimestep.
The checking message on the last commit is not correct, the two Zeldovich Pancake tests still fail. Hopefully fixed soon.
It might be a good idea to activate pipelines testing for your fork soon so that the tests will be run automatically here.
Thanks, @dcollins4096 . Please let us know when you've sorted that out!
Ok, I pushed one last change. NOW all the tests that fail are due to the change in the timing of SetTimeNextTimetep. We should also update the gold standard, to reflect these changes.
If one reverts the change SetTimeNextTimestep change, one gets the same answers as before. So unless more tests that the ones in Brian's comment from March 2nd, this is ready to be merged upon others' approval.
Thanks for the feedback and notes, @ngoldbaum , I added all your suggestions. Apparently I caused more whitespace diffs than I intended, let me know if it would be preferable to back that change out so the diff is neater.
Thanks for everyone's comments and feedback. Let me know if there are any other comments.
Hi @dcollins4096 , I think that this PR is ready to accept! The changes are minor, as you say, and I think they are readily explainable. I'm going to hit 'Approve'.
Following up on this: after one last pass through the test suite, the only tests that fail are the ones that we expect to fail due to the EvolveLevel re-ordering. I'm going to merge this PR and we'll update the gold standard shortly.