1. The Enzo Project
  2. Untitled project
  3. enzo-dev
  4. Pull requests

Pull requests

#277 Merged at 3b1f962
Repository
enzo-dev-mom
Branch
week-of-code
Repository
enzo-dev
Branch
week-of-code

Kinetic Feedback

Author
  1. Christine Simpson
Reviewers
Description

This pull request adds a new feedback method for star particles that injects both kinetic and thermal energy in a 3x3x3 CIC-type stencil around a star particle. The fraction of kinetic energy can either be set as a constant fraction or can vary with resolution and local gas properties as set by StarFeedbackKineticFraction. The energy can either be injected all at once or gradually as is done with the Cen&Ostriker feedback. This feature is controlled by StarMakerExplosionDelayTime. This pull request also includes some docs describing the usage of this new feedback method.

In addition, this pull request includes a new test problem in the run directory called StarParticle that will place a star particle in the center of a box of uniform gas density and will let it do feedback. It is set up for this new feedback method, but can in principle be used for any stellar feedback method.

Comments (41)

  1. Nathan Goldbaum

    Awesome, thanks for PRing this into the main enzo repo! It will make it substantially easier to integrate into enzo-3.0.

    I haven't looked at this in detail yet, but just out of curiosity, you don't deal with feedback zones overlapping with grid boundaries and just arbitrarily cut them off if the particle is at a grid boundary, correct?

    1. chummels

      No, it pushes the feedback zones over away from the edge. One could imagine just truncating it, but then we lose momentum conservation.

    2. Christine Simpson author

      Cameron is correct. Lines 537 - 570 in star_maker3mom.F aim to do this. Cameron and I have discussed enforcing some kind of refinement criterion that would force grids to contain 'young' particles by a certain buffer. But that is a more complicated piece of development and we wanted to get the basic method into the code first.

  2. chummels

    It's awesome that you're including this new test problem along with the Kin FB, but just a couple small things. Right now you have the test problem in run/StarParticle, when it would probably be better not to hang it directly off the top of the run directory. Could you move it to run/Stars/StarParticle or something like that so there is room for other Star test problems? Also, could you include a notes.txt with a 1-2 sentence description of what is going on and how to run the problem like the other tests have? Lastly, would you mind making a StarParticle.enzotest file like the other tests have which essentially just tag the test to be run based on categories. I'd say this should go in the pushsuite and the fullsuite but not the quicksuite, maybe?

    1. chummels

      A tiny little yt script demonstrating that the cloud expands is also helpful for users running the test suite. This works well:

      import yt
      es = yt.simulation('TestStarParticle.enzo', 'Enzo')
      es.get_time_series()
      for ds in es:
          yt.ProjectionPlot(ds, 'x', 'density').save()
          yt.ProjectionPlot(ds, 'x', 'temperature', weight_field='density').save()
      
    1. Brian O'shea

      Followup to my own note: Cameron reminded me that there are some odd artifacts that he is seeing when he runs kinetic feedback in relatively low-resolution cosmological simulations. These need to be thought about a bit before we approve this PR!

      1. chummels

        I've been in contact with Christine Simpson and Greg Bryan about this, and it appears that the problem isn't so much in the feedback but in the "tests" going on in the code that produce the error messages. I'm testing this out and should be able to comment on the effectiveness soon. I promise to test this PR and either approve or have more feedback by next Monday.

  3. Brian O'shea

    Just a quick note on this PR: chummels has looked through it and run some tests, and all issues that have been brought up thus far have been settled to the satisfaction of all parties involved. I haven't heard from anybody who is interested in reviewing this PR, and Greg Bryan and I concur that there have been enough pairs of eyes on it. I'm going to go ahead and accept with 2 approvals. Thanks to all of you that have taken the time to work on this pull request!

      1. chummels

        I think before this is accepted, someone should run enzo with kinetic feedback on in a cosmological zoom run with an L* galaxy and see how much it differs from a normal Cen-Ostriker FB run with the same ICs. This will be an important use of this framework. I tried to do this a couple of months ago, but ran into some issues, which I think are fixed now, but I haven't had time to test since then. I can do this now that I'm back at home after some family issues took me away from research the last few weeks. Can you give me two days to test this before accepting? I'll have comments on here by Tuesday night.

        1. Brian O'shea

          In my opinion, the degree to which the results differ between two star formation methods for L* galaxy formation is somewhat irrelevant to whether or not we should accept this pull request. The code compiles, runs, and passes the test suite; several people have looked at it; it produces plausible, interesting results for cosmological simulations of dwarf galaxy formation; and Christine has resolved the (rather puzzling) merge conflict. In addition, we already have a strong sense that the results may be different in the L* case, because the dwarf galaxy results are different. In what way would the degree of difference (or lack thereof) affect the acceptance of the PR?

          1. chummels

            You wanted three reviewers. I'm back now and can review as the third reviewer.

            Looking at the code is important, and making sure it doesn't break the test suit is important, but testing the new features to assure that they actually work as described when turned on is just as important. Particularly, since the last time I tried to run this infrastructure, it gave problems.

            1. Brian O'shea

              OK, that's fair. A big concern that I have with regards to your suggestion (but didn't express in my last comment) is that well-resolved, cosmological simulations of Milky Way-sized galaxies take a long time. This PR has been on the books for a long time, and it'd be really nice to get this accepted so we can have an Enzo 2.5 release and move forward with our community plans for Enzo. If you think that you can get this done by tomorrow night, great; if not, I think we should accept the PR and move forward.

  4. chummels

    I just ran some tests of the PR, similar to the tests I ran a few months ago, and I'm seeing similar issues to what I saw then. I get the fatal error from the code that the calculated kinetic energy difference doesn't equal what is actually being added to the outgoing gas momentum. I was under the impression that the check that caused this failure had been removed, but it seems to not have been removed.

    For reference, I'm using the tip commit from this PR (d29c062) on a cosmological test run from my enzo scaling suite (parameter file here) (ie 128^3 25 Mpc box with global AMR = 4) on stampede using 64 processors and the intel compiler. I run it from z=100 to z=5 using normal Cen-Ostriker SF/FB (method #1). Then I turn on these Kinetic FB parameters and run it forward, but it fails immediately with this message stderr/stdout.

    This is a very generic use of the code as far as I can tell, and I think that we should not accept the PR until this works. Perhaps I'm just using it incorrectly? Christine Simpson can you see anything obvious I'm doing wrong? Or did you not take out the check?

    1. Christine Simpson author

      The error you pasted is for the total energy, not the kinetic energy. Is the dual energy formalism on? If it is off, I think I understand the problem and can reproduce it in the test problem. I will fix that and push it. If the dual energy formalism is on, I'm not sure and will have to do some more investigating.

      1. chummels

        The dual energy formalism is on in my parameter file, so I'm not sure what is going on. I'll try running your fix to see if it makes a difference, though.

      2. chummels

        I tested the new code you added, and as predicted, it did not affect my results since the dual energy formalism is on in my runs.

        Are there things you can suggest we do to further constrain the nature of this problem?

                  1. Christine Simpson author

                    Cameron has done a number of tests of a cosmological L* galaxy with different parameters. There have been some issues with the special case of 0 kinetic energy injection. We are trying to resolve those now.

  5. chummels

    Christine Simpson , Greg Bryan , and I have spent the last few months extensively testing this PR to assure it works in a variety of situations. In addition to the tests Christine Simpson ran for her paper, it now seems to give good results for global AMR runs of a large cosmological volume (100 Mpc) with a variety of different Kinetic Feedback parameters (StarMakerExplosionDelayTime and StarFeedbackKineticFraction). The only place where we encountered issues sometimes is when setting StarFeedbackKineticFraction = -1, which invokes the Simpson et al 2015 variable kinetic fraction dependent on gas density, metallicity, and resolution. This sometimes gives a failure with calc_dt = NaN errors, that we couldn't track down.

    I think for now, this mode can be considered experimental, and we can try to track the bug down in the future, but this PR has waited long enough. I have included a small note in the docs regarding this issue.