Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fillboundary #1171

Merged
merged 46 commits into from
Aug 3, 2020
Merged

Fillboundary #1171

merged 46 commits into from
Aug 3, 2020

Conversation

bsrunnels
Copy link
Contributor

@bsrunnels bsrunnels commented Jul 22, 2020

This pull request fixes the problem of edge nodes not getting filled properly when filling multiple layers of ghost nodes. This occurs when there are overlapping ghost nodes on a coarse/fine boundary. (See issue #568)

Summary of changes to AMreX:

  • Added a BoxArray::complementIn function that takes a box list and returns the complement box list.
  • Added an additional boolean argument multi_ghost to all FillBoundary functions that gets passed (eventually) to define_fb. In all cases, a default value of false is supplied.
  • Modified define_fb to include the missing ghost nodes in the list of send_tags. (This uses the new complementIn function.)

Summery of changes to LinearSolver/MultiComponent tutorial:

  • Removed the realFillBoundary function, replaced with `FillBoundary(false,true)
  • Modified main.cpp so that it can be compiled and run (non-trivially) in 2D as well as 3D.

Diagram of the problem that this PR fixes:
FillBoundaryIllustration

bsrunnels added 30 commits July 2, 2020 15:53
@bsrunnels bsrunnels requested a review from WeiqunZhang July 29, 2020 02:32
ba_ksnd.grow(1);
const Box dst_bx_ng = (ba_ksnd & (bxrcv_ng + (*pit))) - (*pit);
const BoxList& bl_ng = amrex::boxDiff(dst_bx_ng, vbx_ng);
bl.join(ba.complementIn(bl_ng));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this works for periodic boundary. The points in bl_ng might overlap with valid points periodically. But if you don't have periodic boundaries. We could add an assertion and have this PR merged.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, thinking more about the approach in this PR, I don't think it will scale well because of the line above even without periodic boundaries. I believe it will be slower than doing two communication passes unless the number of component is huge. The two passes can be done as follows. First, a modified FillBoundary is called to fill the second layer (or beyond) of ghost cells with the valid cells and the first layer of ghost cells. Then a normal FillBoundary is called. In the first pass, one can use this BoxArray member function for intersections

    void intersections (const Box& bx, std::vector< std::pair<int,Box> >& isects,
                        bool first_only, const IntVect& ng) const;

Here the effect of ng is to effectively grow BoxArray by ng. Using this, we can treat the first layer of ghost cells as valid cells.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the first pass, the communication can be restricted to sending from first layer of ghost cells to the outer layers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could add an assertion and have this PR merged.

Assertion added.

I don't think it will scale well because of the line above even without periodic boundaries

Because of the ba.complementIn(bl_ng) call?

First, a modified FillBoundary is called to fill the second layer (or beyond) of ghost cells with the valid cells and the first layer of ghost cells. Then a normal FillBoundary is called. In the first pass, one can use this BoxArray member function for intersections

Ok, so in practice, one would pass an argument (say, ng) to FillBoundary, so that the total call would look like

FillBoundary(ng) // modified fill boundary
FillBoundary() // regular fill boundary

The ng argument for the modified FillBoundary call makes its way to define_fb where it is used in the intersections call. Is that you had in mind? If so, I'm happy to give it a try but I'm doubtful that it will work - I found that the bad FillBoundary behavior happened even when the nodes in question were already filled with the proper values.

@WeiqunZhang WeiqunZhang merged commit 4c50993 into AMReX-Codes:development Aug 3, 2020
@bsrunnels bsrunnels deleted the fillboundary branch August 3, 2020 18:58
dwillcox pushed a commit to dwillcox/amrex that referenced this pull request Oct 3, 2020
This pull request fixes the problem of edge nodes not getting filled properly when filling multiple layers of ghost nodes. This occurs when there are overlapping ghost nodes on a coarse/fine boundary. (See issue AMReX-Codes#568)

Summary of changes to AMreX:
- Added a `BoxArray::complementIn` function that takes a box _list_ and returns the complement box list. 
- Added an additional boolean argument `multi_ghost` to all `FillBoundary` functions that gets passed (eventually) to `define_fb`. In all cases, a default value of `false` is supplied.
- Modified `define_fb` to include the missing ghost nodes in the list of `send_tags`. (This uses the new `complementIn` function.)

Summery of changes to `LinearSolver/MultiComponent` tutorial:
- Removed the `realFillBoundary` function, replaced with `FillBoundary(false,true)
- Modified `main.cpp` so that it can be compiled and run (non-trivially) in 2D as well as 3D.

Diagram of the problem that this PR fixes:
![FillBoundaryIllustration](https://user-images.githubusercontent.com/6371567/88218141-d9982980-cc1c-11ea-84f7-194934dfed73.png)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants