#346 Merged at 7a7e812
Repository
ibutsky
Branch
week-of-code
Repository
enzo-dev
Branch
week-of-code
Author
  1. Iryna Butsky
Reviewers
Description
  • Adding SuperNova class for magnetic supernova feedback. Adding resolved issues with internal energy in the Dedner solver (came up in ProblemType_AgoraRestart).

  • I also added a new linked list class (see list.h) after running into many problems including standard library <list> and <vector>

Comments (12)

  1. Brian O'Shea

    Hi @ibutsky , apologies for my delay in running the test suite on this PR. At changeset 4ee14a0 , your PR passes the test suite.

    I haven't had the chance to go through your source code in great detail, but I have two immediate notes:

    1. The files src/enzo/hydro_rk/SuperNovaSeedField.C~ and src/enzo/hydro_rk/SuperNovaSeedField.o can be removed from the repository.
    2. You need to add documentation explaining the new feedback routine and how it works. The docs don’t have to be extensive, but they DO need to describe what the algorithm does and how to use it. You can also refer to your paper and give a brief description. For a recent example, see @cms2166 's write up of the kinetic feedback star formation model, which can be found here. If you based your documentation off that, you’d be in great shape.

    I would appreciate it if @dcollins4096 , @cms2166 , and @chummels could take a look at this PR as well - all of you have experience that can speak to this.

  2. Christine Simpson

    As Brian suggests, a brief description of what this feedback method aims to do would be great. Also, don't forget to document newly added external parameters in the parameters section of the docs.

  3. Brian O'Shea

    I'm following up after way too long... I'm sorry about that, @ibutsky!

    I've followed up with Iryna via email about the documentation, and that's going to need to be written before we can accept the PR. But, there's a more immediate issue - this PR breaks the test suite!

    The changes you have made to Grid_InitializeUniformGrid.C cause about ten different test problems to seg fault. One problem that's obvious is that you're telling the code that the density is numbered DeNum, which is actually the electron density number. Regardless, when I revert Grid_InitializeUniformGrid.C to the pre-changes version, almost everything gets fixed. I suggest you revert all of your changes for that file. While we had originally asked you to clean it up a bit, it's clear that there's something else subtly wrong going on and it breaks the code. That's definitely not desirable behavior!

    The one difference that changing Grid_InitializeUniformGrid.C back to its pre-PR form does not fix is the test problem MHDZeldovichPancake. The error that I get is that the output times are not equal, and also various values are not the same at the ~10% level to << 1% level. This test problem uses the Dedner MHD and DualEnergyFormalism. I've verified that the differences come from the changing of the coefficient defined on line 103 of hydro_rk/Grid_MHDSourceTerms.C from 1.0 to 0.5, which is a bug fix (as pointed out by @pgrete). This change in simulation results is reasonable and, given that it's a bug fix, desirable. So, I am mentioning it here for completeness, but do not change this back - leave it as-is.

    @cms2166 would you mind looking through this PR again? And, @dcollins4096 and @chummels , could you please look through it as well?

  4. Iryna Butsky author

    Thanks for the follow up @bwoshea !

    I appended some documentation to star_particles.rst. I also reverted Grid_InitializeUniformGrid.C to its pre-changes version.

    I also did some more clean-up: I got rid of the parameter SupernovaSeedFieldSigma which is obsolete in the current implementation of magnetic energy. I changed the SupernovaSeedField parameters to be specified in physical units instead of internal code units.

    1. Brian O'Shea

      Hi @ibutsky , thanks for making these changes! But, one problem remains - Grid_InitializeUniformGrid.C is not entirely reverted to its pre-change version, and is still breaking the test suite. The problem is that Enzo is very picky about the order in which fields are initialized, since elsewhere in the code in various problem generators it's setting up field labels, etc. in a particular oder. You're currently initializing total and internal energy after the velocity components, whereas you should be doing it before, so that's breaking various pieces of Enzo elsewhere. If you go down to Grid_InitializeUniformGrid.C below, and then select "Side-by-side diff", you will see what I mean. If you move lines 56-58 up to right after where you create the density fields (i.e., so that those three lines start at line 50 instead) it should fix everything.

      Also, my dear colleagues seem to be asleep at the switch with regards to reviewing PRs... I'll send out an email to the mailing list to ask for volunteers to look at the PR once you've made that update and it passes the test suite!

      1. Iryna Butsky author

        Sorry about that! I reverted Grid_InitializeUniformGrid.C to changeset: 6067:df6c1d649319 from October 10, 2016. I think it should run now!

        1. Brian O'Shea

          It does run now, and passes all tests. However, you took out the one chunk of code that SHOULD be in there...

              if (HydroMethod == MHD_RK){
                for (i = 0; i < size; i++)
              BaryonField[PhiNum][i] = 0.;
              }
          

          Mind putting that back in? And sorry for the churn on this... we want to make sure to get it right! :-)

              1. Brian O'Shea

                ok, thanks to @cms2166 and @dcollins4096 for signing off. I'm going to approve and merge. Nice job, @ibutsky !