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

Pull requests

#274 Merged at f68dcef
Repository
enzo-2.x-larrue
Branch
week-of-code
Repository
enzo-dev
Branch
week-of-code

Bug on right side of grid::CheckForExternalReflections

Author
  1. James Larrue
Reviewers
Description

Fixed bug where the right side was filled from the grid's active region outwards, rather than from the edge of the domain. This implementation will start on the right edge of the grid (i.e. the last ghost zone) and fill inwards as far as needed.

The left side was already implemented correctly, starting on the left edge of the grid (i.e. the first ghost zone) and filling inwards as far as needed.

I suspect changing the order of the for-loops would make things faster. If the reviewers think so too, I can change them all to be k, j, i (column-major) order.

Comments (12)

    1. James Larrue author

      The code in question is called if (a) the boundary is reflecting and (b) the ghost zones of the subgrid extend beyond the domain edge.

      On the left side, imagine we have the following situation:

      Domain = [0.0, 100.0] (Below, '|' represents the domain edge)
      Subgrid = [1.0, 11.0]
      CellWidth = 1.0
      NumberOfGhostZones = 3
      
      Domain Units    -1.5  -0.5 |  0.5   1.5   2.5   3.5    ...   8.5    9.5  10.5  11.5  12.5  13.5  ...
      Subgrid Indices   0     1  |   2     3     4     5     ...    10    11    12    13    14    15
      Ghost or Real     G     G  |   G     x     x     x     ...     x     x     x     G     G     G
      

      The code calculates the GridLeftOffset as:

      GridLeftOffset = nint( (GridLeftEdge - DomainLeftEdge)/CellWidth )
                     = 1
      

      Since the subgrid extends beyond the domain, i.e. GridLeftOffset < GhostZones, the reflecting BC will be applied for (i=0; i < (3-1); i++) :

      i=0: Copy From 5 -> To 0
      i=1: Copy From 4 -> To 1
      

      This is correctly filling in values for subgrid cells that are beyond the domain.

      Now imagine a different subgrid, located on the right edge of the domain:

      Domain = [0.0, 100.0] (Below, '|' represents the domain edge)
      Subgrid = [90.0, 99.0]
      CellWidth = 1.0
      NumberOfGhostZones = 3
      
      Domain Units    87.5  88.5  89.5  90.5  91.5  92.5    ...   96.5  97.5  98.5  99.5 | 100.5 101.5
      Subgrid Indices   0     1     2     3     4     5     ...    10    11    12    13  |  14    15
      Ghost or Real     G     G     G     x     x     x     ...     x     x     x     G  |   G     G
      

      The code calculates the GridRightOffset as:

      GridRightOffset = nint( (DomainRightEdge - GridRightEdge)/CellWidth )
                      = 1
      

      Since the subgrid extends beyond the domain, i.e. GridRightOffset < GhostZones, the reflecting BC will be applied for (i=0; i < (16-12-1-1); i++) :

      i=0: Copy From 12 -> To 13
      i=1: Copy From 11 -> To 14
      

      The issues are that:

      1. cell 15, which is beyond the domain, was not assigned a value, and
      2. cell 13, which is inside the domain, was assigned a value.

      The old code did not correctly fill in values for subgrid cells that are beyond the domain (i.e. it missed cell 15, but assigned a value to 13).

      The modified code should apply the reflecting BC for:

      i=0: Copy From 10 -> To 15
      i=1: Copy From 11 -> To 14
      

      The modified code should correctly fill in vaules for subgrid cells that are beyond the domain.

    1. Brian O'shea

      dcollins4096 , there are several test problems that use this code. The Athena Rayleigh-Taylor test, the Free Expansion tests, the Rayleigh-Taylor MHD test (featuring CT!), and the RadiatingShockLab_sp FLD test. There might be others, but I note these ones primarily because they are broken according to Jenkins. :-)

  1. Brian O'shea

    According to Jenkins, there are 82 failures in the test suite:

    http://srs.slac.stanford.edu/hudson/job/enzo-dev/240/

    Looking at this in some more detail in the test report (http://srs.slac.stanford.edu/hudson/job/enzo-dev/240/testReport/) these are unsurprisingly all in test problems that use reflecting boundary conditions along at least one dimension. An inspection of a bunch of the failed tests (http://srs.slac.stanford.edu/hudson/job/enzo-dev/240/testReport/) show that this is largely due to substantial floating point differences between pre-PR and post-PR results (at the 1% level or greater on a cell-by-cell basis for the Rayleigh-Taylor tests, but much smaller for the other tests). I'm pretty sure that I understand all of the differences, and that the new code is more correct, but I need to look at the simulation outputs more thoroughly before accepting the PR. Expect results soon!

    1. Brian O'shea

      OK, following up on my own comment: I have looked into the changes, and they're all very minor and clearly due to this bugfix. I think that we can accept this PR.