Last time I issued a pull request, it was sitting there for a long time, and I felt that it got behind Enzo-dev again. So I canceled the pull request, and now I am redoing everything and re-issuing a pull request.
There is really not much change that I made. Nothing intrusive. Dear Enzo developers, if you have a few minutes, please take a look. Thank you.
Also, I fixed some minor problems with the ShockInABox test. The initial parameters were not right for some reason.
Hi @yl2501 , I am on vacation this week and will push this along next week when I return. Would you like to suggest some reviewers?
Thank you, Brian! I suppose Greg can do it too when he is back (next week?). Though last time it was also the two of you who looked at it. Do we need a third person? Anybody can do it really. There is nothing fancy or complicated here. I just want it done so that I do not have to worry about merging for a while, because I am so bad at it!
Maybe John Wise or David Collins? I can beg them.
No need to beg! I can review it early next week.
Thank you, John!!
Thanks for the PR Yuan! I will review it early next week.
This looks good to me -- I approve! The only (very minor) comment I have is that it would be nice to add documentation for the two new uses cases for the parameter ClusterSMBHCalculateGasMass.
I have two small things:
Why did the velocity values change in the ShockInABox?
Could you also add a note in the docs about the new behavior of PointSourceGravity==2 when ProblemType == 108?
Other than that I think it's fine.
I will approve after addressing my very minor comment in the code and adding docs. Also, I'm curious about the velocity changes in ShockInABox like @dcollins4096.
Thank you, everyone!
I have updated the doc following David's and Greg's suggestions, and modified Grid_ClusterInitializeGrid.C following John's suggestion.
When I played with ShockInABox recently, I found that with the default velocity, the box is not moving with the shock at the right speed (the shock is supposed to be at a fixed location but is actually moving in this case). If I generate the parameters using make_shock.py (with the default Mach number 3), they are different than the default parameters. The new set of parameters gives a shock that is not moving, so I updated ShockInABox.enzo with the newly generated parameters.
Just in case I made a mistake, maybe someone can run the ShockInABox test to verify?
@yl2501 , you have three approves for this PR. You do not have the test suite pipeline set up on your BitBucket account, so I have to do it by hand. I'll run the test suite (which runs the ShockInABox) and will accept assuming everything passes!
Following up on this, all tests past except for ShockInABox, which (predictably) has a bunch of test failures. When I looked at the outputs, I see the following at the end of the simulation:
The original (pre-PR) version, which shows a shock that has moved from x=0.5 and some spurious oscillations:
The new (Yuan's PR) version, which has a stationary shock and some oscillatory behavior that is making yt's shock finder do some interesting and clearly wrong things:
Anyway, I think that this change is a good one and the broken tests represent a bug fix. My only request for @yl2501 is to make the small change I suggested to make_plots.py, which will make the script compatible with both Python 2 and 3 (the removed parentheses were a Python 3 update). So, if you can make that small tweak I'd be happy to merge the PR!
Thank you, Brian! Yes, I forgot to change it back to python3. Now I have updated the PR with the python3 print.
As far as I understand, the oscillatory behavior, if you are talking about the little spikes in the lower left panel, is not from yt. I do not think that yt has a shock finder (please let me know if it does now!). The yt field "mach_number" is the gas velocity divided by sound speed, but what is plotted here is "Mach", which is the mach number computed from Enzo's shock finder (written by Sam?). The shock finder is finding some very small oscillations that originate from the mismatch between the analytical solution and the numerical solution in the initial condition. They are identified as extremely weak shocks with Mach numbers very close to 1. If you use the Zeus method, then these oscillations become large enough to show up in the temperature and density profiles.
Thanks for updating the code.
Regarding the little Mach oscillations, that's very interesting and I'm surprised at the behavior. (Sam wrote the shock finder, but we worked together very closely on it.) Anyway, I think it's a problem that we won't be solving any time soon, and M ~ 1 shocks are always suspect using these methods anyhow.
I'm going to accept and merge this PR. Thanks for your patience in putting it into the codebase!
At some point in the future it might be helpful to put a little note somewhere maybe in make_plots.py, so that general users are not confused by the little Mach oscillations. Though the fact that this has never been brought up before probably means nobody has looked at this test for a while. Maybe at the next Enzo workshop some people can spend a bit of time going through the tests to make sure the default parameters and default **.py all make sense. Just an idea :).