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

Fix find active cell around point. #10174

Merged
merged 2 commits into from
May 15, 2020

Conversation

luca-heltai
Copy link
Member

@luca-heltai luca-heltai commented May 13, 2020

Working on step-70, we hit this bug which makes running particle based applications impossible on large clusters.

I'm not sure yet hat is wrong. For the moment, this is a failing test. The (correct) output is generated using line 46.

@gassmoeller @blaisb

This is one of the two bugs we uncovered with step-70.

My educated guess is that the version that uses a Cache object also does things differently, like passing a tree of locally owned vertices, and therefore it fails to find the points when they are on artificial cells.

I'm going to bed... tomorrow I'll try to see if I can solve this.

tests/grid/find_active_cell_around_point_04.cc Outdated Show resolved Hide resolved
@bangerth bangerth added this to the Release 9.2 milestone May 13, 2020
@luca-heltai
Copy link
Member Author

This test currently fails. I'm trying to figure out a fix.

@luca-heltai
Copy link
Member Author

luca-heltai commented May 14, 2020

The fix is dirty: I did not understand what is wrong with the actual algorithm. This algorithm was developed by @gassmoeller and myself during a deal.II developer workshop, and it seems to work most of the times, but sometimes it fails. I have not understood why, so in the fix I revert to the other algorithm when things are failing. The other algorithm is less smart and much slower, but seems to do the job correctly.

@luca-heltai
Copy link
Member Author

/rebuild

@bangerth
Copy link
Member

Hm, but then we have a regression-of-sorts. That's disappointing as well. We should really try to find out what the actual problem is...

@luca-heltai
Copy link
Member Author

Hm, but then we have a regression-of-sorts. That's disappointing as well. We should really try to find out what the actual problem is...

I agree. But there are two bugs now in the way of step-70. I'd like to try and fix the other one as well, and see if we manage to get going. At least now the behavior is correct. We should keep the issue open, even if we decide to merge this.

@bangerth
Copy link
Member

In order to assess how to move forward: What's the other bug you run into?

@luca-heltai
Copy link
Member Author

@bangerth : #10211

@luca-heltai luca-heltai mentioned this pull request May 15, 2020
@tamiko
Copy link
Member

tamiko commented May 15, 2020

@bangerth Please re-review and accept if appropriate. I would like to get this into rc2.

@tamiko tamiko changed the title Failing test on find active cell around point. Fix find active cell around point. May 15, 2020
@kronbichler
Copy link
Member

@bangerth let us move forward here. We have time later to understand why the slow path is needed.

@kronbichler kronbichler dismissed bangerth’s stale review May 15, 2020 05:57

The new commit fixes it and we need to move forward.

@kronbichler kronbichler merged commit de37828 into dealii:master May 15, 2020
kronbichler added a commit that referenced this pull request May 15, 2020
@bangerth
Copy link
Member

@luca-heltai -- can you open a PR that reminds us that we need to investigate this?

@kronbichler
Copy link
Member

@bangerth we have #10174 open. Let me put the description of what we should improve to that PR and adjust the target milestone to 9.3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants