#401 Merged at 236b238
Repository
enzo-dev-HMinus
Branch
week-of-code
Repository
enzo-dev
Branch
week-of-code
Author
  1. John Regan
Reviewers
Description

This PR includes the addition of H^- and H2II photodetachment to the RT network. I've also modularised the different photon reactions into their own files.

Comments (26)

  1. Brian O'Shea

    Hi @john_regan , a quick note: I wanted to double-check that this PR passed the test suite with the tip of enzo-dev (the gold standard is a bit behind the tip right now, I believe). It turns out that it fails 44 tests against the tip of enzo-dev when I use the --strict=high setting in the test runner, with errors in the PhotonShadowing and (mostly) PhotonTestAMR test problems. Turning the standard down to --strict=low (which accepts differences within 1 part in 1000) still has 34 failed tests. This indicates that something actually IS difference, and needs to be looked into a bit more!

    1. John Regan author

      Yes I did the same and noticed the same. I checked the test results and they are identical so the differences must be extremely small. Nonetheless you are correct it is failing. The PR replaces some hardcoded values with parameters (e.g. the distance that a ray is allowed to travel) and they are set differently to the enzo-dev case (sqrt(3) as opposed to 1). I'll take a closer look at this next week to see where exactly the differences are coming from -(most likely the parameter files for the tests need to be adjusted to match the enzo-dev result exactly).

    2. Britton Smith

      I'll issue a PR to update the gold standard so it'll display here which tests are failing. We should be updating the gold standard every time we accept a PR that changes the answers anyway.

  2. John Regan author

    Hi @bwoshea ,

    I committed some minor changes and the tests are now passing on my local machine. They still fail using the pipeline but this may be because the gold standard is behind? The main changes are in Grid_WalkPhotonPackage.C. Cheers, John

    1. Brian O'Shea

      Hi @john_regan , that's probably right. We've just updated the gold standard, but I will run the tests by hand and verify that things are behaving as expected!

      1. Brian O'Shea

        OK, following up on this - I've run the test suite by hand vs. the current tip of the week-of-code branch, and all tests pass. I think that we should now rond some people up to look at the code and give feedback, but it's ready to go as far as the test suite is concerned!

        1. John Regan author

          Awesome! I've purposely left a #define in RadiativeTransferIonization.C which indicates what caused the differences. @jwise77 would be the best person to comment on its value or otherwise though. The code review can determine whether it lives or dies.

          1. John Regan author

            Hi @bwoshea ,

            Can you poke some referee's on this code review? I have a couple of more ones coming down the tracks so want to get this one moving. Thanks!

  3. Brian O'Shea

    As of this morning (May 1), this PR passes the test suite (push suite), as compared to the tip of enzo-dev.

  4. Brian O'Shea

    I looked through, and have very minor comments on the code - please see below. But, I do not see any mods to the documentation. While this isn't really affecting any user parameters, it's worth noting somewhere that the new physics is in the code, don't you think?

    1. John Regan author

      I've pushed updates to most of your comments below. The only one I didn't / can't change is the clight comment. Using clight before caused very slight changes to the outputs but was enough to break the tests. I'll do the updates to the docs now.

      1. Nathan Goldbaum

        Why don't you use the phys_constants.h clight and then update the gold standard before merging the PR?

      2. Brian O'Shea

        I think it's fine to break the tests in this way - it's really easy to update the gold standard. That said, it's hardwired in a bunch of different places in the code (to various values that are all slightly different), so I don't think there is much harm in leaving it as-is.

        1. John Regan author

          I think I would prefer to use 'clight'. I wanted initially to get to a place where the PR was passing the tests and that required not using clight. I could have used clight and the tests would break but saying it was because clight is used would have sounded very ropey! If everyone is happy with the changes and the tests are passing I'm happy to switch to clight and then merge.

  5. John Wise

    This is a pretty substantial and needed enhancement to the ray tracer. I read through the changes, and they all look good to me. I paid special attention to the change of the CrossSection array to a scalar, and I think it makes sense to only store one value if they are calculated on the fly. Also, I like that the photo-ionization/heating calculations are now in their own routines. I worry a little about the efficiency (i.e. vectorization) of the ray trace in Grid_WalkPhotonPackage.C with all of the changes, but we can check any performance differences and possible improvements at a later date. The important bit is that the PhotonTest results remain virtually unchanged. I'm approving.

    1. John Regan author

      Great - thanks @jwise77 . I'm curious about your analysis of the vectoriation issue. Do you think the additional function calls will hurt the vectorisation?

  6. Greg Bryan

    This looks great -- I'm also happy with it and am approving (although didn't look at quite the same level as John). My one very minor thought is that the documentation would be helped if the parameter RadiativeTransferH2Shield was changed to RadiativeTransferH2ShieldType (to differentiate it from an on/off switch). Also, is there a place in the physics description of the documentation where it could be noted that this exists. But these are minor and happy to approve even without. Thanks!

    1. Brian O'Shea

      @john_regan , would you mind commenting on this? I think that @gbryan 's suggestion about RadiativeTransferH2ShieldType makes a lot of sense, as well as a documentation tweak. Please let us know how you want to proceed!

  7. John Regan author

    @bwoshea I think both of @gbryan 's are very valid I'll definitely update the H2Shield parameter so let me do that before merging. For the documentation I don't see a place where a description of the additional physics obviously goes. Maybe we need a section in the docs that addresses the physics available in Enzo both on the grid (e.e. ray tracing) and subgrid (e.g. PopIII formation)? Or maybe it already exists.

    1. Brian O'Shea

      @john_regan ok, I'll hold off on merging until you update the parameter. Regarding the docs, you're right... I think that we probably need to have a list of all of the physics in the code. This is definitely something to discuss at the Enzo workshop in a week and a half!

      1. John Regan author

        OK that all sounds good. I'll update the parameter shortly and push the changes. For the docs I think a description of some of the main physics might be a nice addition to the docs. Like you said a good topic for discussion at the workshop.

  8. Brian O'Shea

    As with PR #364, I ran the test suite one more time after @john_regan 's last set of commits. The tests that fail are the ones that we expect to fail due to the clight fix. I'm going to merge this PR and we'll update the gold standard soon.