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.

Sign up for GitHub

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

Jump to bottom

First design of MD capability in Magpie #369

Merged
merged 1 commit into from
Feb 13, 2019
Merged

First design of MD capability in Magpie #369

merged 1 commit into from
Feb 13, 2019

Conversation

snschune
Copy link
Contributor

@snschune snschune commented Jan 22, 2019 •
edited by dschwen
Loading

Refs #367

@snschune snschune requested a review from dschwen January 22, 2019 23:51
@snschune snschune force-pushed the get_started_on_lammps_367 branch 2 times, most recently from aa73963 to 07b333b Compare January 23, 2019 00:00
@moosebuild
Copy link

Job Test on 07b333b : invalidated by @dschwen

KDTree commit should be in MOOSE now

@dschwen
Copy link
Member

dschwen commented Jan 23, 2019

Nope, not in master yet...

dschwen
dschwen reviewed Jan 23, 2019
View reviewed changes
src/userobjects/MDRunBase.C Outdated

#include "libmesh/mesh_tools.h"

// custom data load and data store methods for a class with virtual members (vtable pointer must not
Copy link
Member

Choose a reason for hiding this comment

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

This comment is not appropriate here. We need the custom load/store because we have members with data on the heap.

src/userobjects/MDRunBase.C
std::vector<std::pair<std::size_t, Real>> indices_dist;
BoundingBox bbox = elem->loose_bounding_box();
Point center = 0.5 * (bbox.min() + bbox.max());
Real radius = (bbox.max() - center).norm();
Copy link
Member

Choose a reason for hiding this comment

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

Why not use centroid and h_max here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to make sure to construct a sphere that contains the element. h_max is the spacing between nodes and I am not sure that h_max / 2 is equal or larger than the radius of the circumscribing sphere.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, you're right. I may have made this mistake already somewhere in the code... :-/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The alternative is to compute the circumscribing sphere in Elem. It requires computation of the centroid though and that might be expensive. I could push a corresponding function in libmesh.

Copy link
Member

Choose a reason for hiding this comment

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

No, let's not go down that rabbit hole just now. The centroid is not necessarily the center of the circumsphere (circumcenter). This is not trivial.

https://math.stackexchange.com/questions/1087011/calculating-the-radius-of-the-circumscribed-sphere-of-an-arbitrary-tetrahedron

@snschune snschune force-pushed the get_started_on_lammps_367 branch from 07b333b to 6f9c9ae Compare January 24, 2019 22:58
@moosebuild
Copy link

moosebuild commented Jan 24, 2019 •
edited
Loading

Job Test on 4230e08 wanted to post the following:

View the site here

This comment will be updated on new commits.

@snschune snschune force-pushed the get_started_on_lammps_367 branch 2 times, most recently from 2822d60 to dacc36a Compare January 30, 2019 21:01
@moosebuild
Copy link

Job Precheck on dacc36a wanted to post the following:

Your code requires style changes.

A patch was auto generated and copied here
You can directly apply the patch by running, in the top level of your repository:

curl -s http://mooseframework.inl.gov/magpie/docs/PRs/369/style.patch | git apply -v

Alternatively, with your repository up to date and in the top level of your repository:

git clang-format 80c406d191dbaa68087d784a79ee6d8ccf43b8fc

@snschune snschune force-pushed the get_started_on_lammps_367 branch 2 times, most recently from ffef8c1 to 63aa6d3 Compare January 30, 2019 22:46
@snschune
Copy link
Contributor Author

Algorithm double counts MD particles on faces that are also processor interfaces.
I prevent double counting on faces on each processor.

@dschwen
Copy link
Member

dschwen commented Jan 31, 2019

Is this ready then?

@dschwen
Copy link
Member

dschwen commented Jan 31, 2019

Algorithm double counts MD particles on faces that are also processor interfaces.
I prevent double counting on faces on each processor.

Ah, I see, no double counting on element boundaries unless they also are processor boundaries. :-/
How is this fixed in Dirac Kernels? I suspect a similar problem exists there.

@snschune snschune force-pushed the get_started_on_lammps_367 branch from 63aa6d3 to d238e85 Compare January 31, 2019 19:56
@snschune
Copy link
Contributor Author

I have implemented it differently. Loop over all semi-local elements and accept particles into the element with the smaller element unique id.

@snschune snschune force-pushed the get_started_on_lammps_367 branch 3 times, most recently from f1943a7 to 09f9b75 Compare February 1, 2019 18:37
@snschune snschune force-pushed the get_started_on_lammps_367 branch from 09f9b75 to 4230e08 Compare February 1, 2019 20:39
dschwen
dschwen approved these changes Feb 13, 2019
View reviewed changes
Copy link
Member

@dschwen dschwen left a comment

Choose a reason for hiding this comment

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

This looks good. In the future we might want to allow reading proper lammps dump files (not just .xyz). I think I have code for that already.

@dschwen dschwen merged commit 37bcc7e into idaholab:devel Feb 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dschwen dschwen dschwen approved these changes

Assignees
No one assigned
Labels
ready for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@snschune @moosebuild @dschwen