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

Change sharedtria dof distribution to match distributed tria #5436

Merged
merged 1 commit into from Nov 20, 2017

Conversation

tcclevenger
Copy link
Contributor

This was discussed here: #3078 (comment)
Talking with @tjhei, we felt it best to change from "coin flip" assignment to "smallest subdomain" assignment for those active dofs on the interface between two subdomains of a p::shared::T so that we match the distribution in p::distributed::T (this is also how we chose to distribute mg dofs for p::shared::T).
Note:

  • There are 4 different tests (16 total runs) that we expect to fail with this change. This is due to the fact that each of these test output n_locally_owned_dofs. This value will obviously change when we change the way in which we break ties along the interface.
  • I have added two tests, i) testing against a p4est mesh for accuracy against p::distributed::T and ii) creating an adaptive mesh and outputting the owned dofs for each subdomain and dof numbering for each cell.
  • I changed some filenames from tests I wrote for a previous pull request. Just a wording change.

@davydden
Copy link
Contributor

/run-tests

@@ -0,0 +1,14 @@

{[0,20]}
Copy link
Member

Choose a reason for hiding this comment

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

you need to put p4est into the filename

@agrayver
Copy link
Contributor

For the record: DoFTools::subdomain_wise (or, more specifically, DoFTools::get_subdomain_association) also uses the coin flip strategy and this patch does not change it (either intentionally or not).

@tjhei
Copy link
Member

tjhei commented Nov 10, 2017

(either intentionally or not).

Yes. I was thinking about this yesterday. Those functions can go away. I will make a PR that deprecates them.

@bangerth
Copy link
Member

This would deserve a changelog entry in the "incompatibilities" section.

@tcclevenger
Copy link
Contributor Author

I'm a bit curious as to why the checks actually passed considering I have not yet fixed the test which fail to pass on my computer due to the change. Looking at the build details it doesn't appear to have run any sharedtria tests. I just still go back and update these, correct?

@bangerth
Copy link
Member

Yes, you need to update these anyway. The tester only runs a subset of the sequential tests because running the entire test suite would take too long.

@davydden
Copy link
Contributor

I have not yet fixed the test which fail to pass on my computer due to the change

let us know when you check locally that all relevant tests pass with this PR. I put a WIP tag for now...

@tcclevenger
Copy link
Contributor Author

All tests for sharedtria now pass.

@davydden davydden removed the WIP label Nov 11, 2017
@tcclevenger
Copy link
Contributor Author

I've added the changelog entry and all relevant test have passed. Let me know if there are any other changes needed, if not everything should be good to go. Thanks!

@jppelteret
Copy link
Member

@tjhei @davydden @kronbichler Can this be merged?

@davydden
Copy link
Contributor

I have no further comments, ok to merge as far as I am concerned.

@tjhei
Copy link
Member

tjhei commented Nov 20, 2017

I will go ahead and press "merge" then.

@tjhei tjhei merged commit e224efc into dealii:master Nov 20, 2017
@tcclevenger tcclevenger deleted the distribute_dofs_shared branch November 21, 2017 14:02
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

7 participants