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

Pull requests

#158 Merged

Ray tracer bug fixes for isolated BCs & constant dt

  1. John Wise

The code tests for the Enzo paper uncovered a couple of bugs in the ray tracing routines. The first occurred in runs with isolated BCs when the subgrid marker was being communicated and caused seg faults. The second occurred in runs with load balancing and a constant RT timestep that caused mismatches in photon counts on each grid and caused warning messages.

This PR fixes both of these bugs.

Update: changed PhotonTest parameter file so the source is on from the very beginning of the problem.

Comments (16)

  1. Brian O'shea

    Hi John,

    This looks reasonable to me. I'll run the method paper test on a couple of different systems and let you know how it goes. Also, does this pass the push suite?


  2. John Wise author

    An update for everyone... I found that the SubgridMarker was not being communicated correctly when RT load balancing is used. I've written a fix, and I'm testing it now.

  3. John Wise author

    I've tracked down the bugs that were causing the missing photons in the test, and I've updated this PR. The variations in HI_kph in the x-slice are caused by inherent artifacts from the ray tracer and does not affect the ionization state of the gas significantly. Here are the slices from the final output of the test. http://imgur.com/a/TwV9x#0

    I think it's good to go.

  4. Brian O'shea

    Hi John,

    I also do not see any artifacts in the PhotonShadowing test. When I run on the other radiation transport tests (in particular, run/RadiationTransport/PhotonTest), I do not get the right answer in terms of the propagation of the Stromgren sphere. This image contains the answer that comes along with the code checkout: http://i.imgur.com/aryuXBB.png . Note that the position of the I-front is always within a few percent of the analytic position. Using the new code, I get this: http://i.imgur.com/Af1oMrL.png . Note that the error at early times is enormous, and the shape of the curve is not what the analytic result would predict it to be. I'm not sure if this is due to the radiation transport code getting modified, the test problem getting modified, or what, but there definitely appears to be a problem beyond the artifacts we were seeing.

  5. Brian O'shea

    Hi John,

    Actually, just to follow up: I tested all the way back to enzo-stable (v2.2, Nov. 2012) and it gives precisely the same answer as the current version of the code in terms of the I-front location for the PhotonTest problem. So, whatever is going on here, it's not introduced in this bug fix - it has been in the code since at least this past fall. Alternately, there's something wrong with the compilers on the machine that I am using to test it (iMac with OS X 10.8, gcc 4.7 stack).

    It would be very useful if somebody else could verify this bug - in the meantime, I'll keep digging. Thanks!

  6. Brian O'shea

    Sorry for the barrage of emails. I've learned that when I set RadiativeTransferAdaptiveTimestep and RadiativeTransferHIIRestrictedTimestep to 0, the results become substantially better - basically back to the expected values, although the simulation takes about two orders of magnitude longer. When I set RadiativeTransferAdaptiveTimestep = 1, RadiativeTransferHIIRestrictedTimestep = 0, things go insane (the I-front position as a function of time is utterly off, and wildly too small) and when I flip those values (RadiativeTransferAdaptiveTimestep = 0, RadiativeTransferHIIRestrictedTimestep = 1), everything looks fine. (I appreciate that last combo is explicitly discouraged in the documentation, but I wanted to see if it worked.) Anyway, I thought that might provide some useful data!

  7. John Wise author

    Thanks for pointing this out, Brian. I'll see if I can find out what's causing the Strömgren radius not to match the analytical result.

  8. John Wise author

    Whew... it wasn't anything wrong with the code. The parameter file was incorrect, where the radiation source wasn't being turned on until the 2nd timestep just before 10 Myr. I'll update the PR shortly.

  9. Brian O'shea

    OK, I've verified that it now produces results that are identical to the previous standard, and are very close to analytic. Thanks for the quick turnaround, John!

  10. Nathan Goldbaum

    Hey John,

    I'm getting identical results compared to Brian on a linux cluster, so I think this is ready to be merged in. My only comment (besides the conflict that needs to be resolved due to a change Sam made in his PR) is that the png images that get distributed with the ray tracing test problems should probably be updated since they are seriously out of date. It looks like they were produced in 2010 or so, before a number of bugfixes were submitted.

    1. John Wise author

      On second thought after looking at the images this morning, I don't think the results have changed enough to affect the images to necessitate new ones. The standard may have to be updated, but a human can't notice the differences in the images.

  11. Brian O'shea

    OK, I agree with you about the images and the plot of I-front as f(t). One thing that I forgot: when I run scripts.py in the PhotonTest directory, it dies on this line:


    If I change it to:


    it works fine and produces the correct image. Could you update this? After that, I think it's ready to merge.