Eccentric hyper shell #8526

melaniegerault · 2019-08-08T23:30:21Z

This is an addition to GridGenerator to make an eccentric hyper shell, where the centers of the two circles (2D) or spheres (3D) are shifted.

drwells · 2019-08-08T23:31:18Z

Nice! Do you have a picture?

drwells · 2019-08-08T23:32:01Z

include/deal.II/grid/grid_generator.h

+   *
+   * A SphericalManifold is attached to the outer boundary with an id of 1 while
+   * another SphericalManifold is attached to the inner boundary with an id of
+   * 0. A TransfiniteInterpolationManifold is attached to all other cells and


tamiko · 2019-08-08T23:32:25Z

source/grid/grid_generator.cc

+    // the original hyper_shell function assigns the same manifold id
+    // to all cells and faces. Set all manifolds ids to a different
+    // value (2), then use boundary ids to assign different manifolds to
+    // the inner (0) and outer manifolds (0). Use a transfinite manifold


Would you mind fixing the typo?

drwells

Nice work. Would you please also add pictures of the grid in 2D or 3D? You can upload them to GitHub and the principal developers can take it from there.

drwells · 2019-08-08T23:33:16Z

source/grid/grid_generator.cc

@@ -3004,6 +3003,58 @@ namespace GridGenerator
    tria.set_manifold(0, SphericalManifold<2>(center));
  }

+  template <int dim>


Please use three blank lines between definitions.

This should also be resolved in the last commit. Sorry, I don't know how show the update.

source/grid/grid_generator.cc

drwells · 2019-08-08T23:34:41Z

source/grid/grid_generator.cc

+    std::set<Point<dim> *> vertices_to_move;
+
+    for (const auto &cell : tria.active_cell_iterators())
+      for (unsigned int f = 0; f < GeometryInfo<dim>::faces_per_cell; ++f)


You could use active_face_iterators instead to combine these two loops.

drwells · 2019-08-08T23:36:27Z

include/deal.II/grid/grid_generator.h

+   * in radial direction to form 6 cells), 12 for the rhombic dodecahedron,
+   * and 96. This choice dates from an older version of deal.II before the
+   * Manifold classes were implemented: today all three choices are roughly
+   * equivalent (after performing global refinement, of course).


I think it would be clearer (and future-compatible) to instead write that n_cells has the same meaning in this function as it does in hyper_shell.

This comment and the comment above should be resolved by my last commit, but I don't know how to bring the updated versions of the .cc and .h files in here!

bangerth · 2019-08-09T03:21:03Z

source/grid/grid_generator.cc

@@ -146,7 +146,7 @@ namespace GridGenerator
      // data, we do this only based on
      // topology.

-      // For the mesh based on  cube,
+      // For the mesh based on cube,


ha, nice catch!

bangerth · 2019-08-09T03:23:21Z

source/grid/grid_generator.cc

+
+  template <int dim>
+  void
+  eccentric_hyper_shell(Triangulation<dim> &tria,


I think you actually copied the same function a second time here. If correct, you might want to remove it!

Indeed! The second one was actually the correct one.
Do I need to add my new version of the file here if I already pushed a new commit with it on my fork?

melaniegerault · 2019-08-09T05:41:54Z

Nice! Do you have a picture?

I should have pushed two png files into the image folder of the doc (called eccentric_hyper_shell_2D.png and eccentric_hyper_shell_3D.png). Otherwise, this is an attempt to attach them here!

drwells · 2019-08-10T18:18:19Z

I see that the pictures are in the repository. Would you please amend the documentation of the function to link to those pictures? We do this for most of the GridGenerator functions. Feel free to ask if you need some help in doing this!

bangerth · 2019-08-13T20:43:23Z

Nice work!

/rebuild

bangerth · 2019-08-13T20:43:55Z

@tamiko -- would you mind lifting your block?

melaniegerault · 2019-08-13T20:53:49Z

I see that the pictures are in the repository. Would you please amend the documentation of the function to link to those pictures? We do this for most of the GridGenerator functions. Feel free to ask if you need some help in doing this!

I might need help indeed, because I'm worried I screwed up something with GitHub! I am stuck. git status tells me that "Your branch is ahead of 'origin/eccentric_hyper_shell' by 43 commits." and I have also seen my name involved in pull requests that I have nothing to do with. So before I can make the change, I have to figure out what I did wrong in git! Hilfe! :)

Besides, @tamiko pointed out that my figures were quite heavy with respect to the other ones in the documentation, so I made some new, less good looking ones. I attach them below. I have removed my old ones from the documentation, but I need to sort my git situation before I can push these new ones! Thanks a lot!

melaniegerault · 2019-08-13T20:58:11Z

@tamiko -- would you mind lifting your block?

I think the block is from @drwells because I haven't been able to include the link to the figures in the documentation yet (see above). I don't want to push my stuff until I figure out if I'm doing something wrong with git!

drwells · 2019-08-13T21:02:47Z

Its me, not Matthias.

We need to do two things before we merge:

clean up the history slightly (we got some extra merge commits; we should not have commits from @tamiko on this branch)
Add the images inline, e.g.,

* @image html eccentric_hyper_shell_2d.pg

or whatever we called it.

Add an eccentric hyper shell geometry that is formed by two spheres with different center points.

Co-Authored-By: David Wells <drwells@email.unc.edu>

tamiko · 2019-08-14T17:41:54Z

@drwells I have rebased the pull request. Please finish your review and merge :-)

drwells · 2019-08-14T17:43:24Z

Let me take a look at the generated doxygen.

melaniegerault · 2019-08-14T17:45:33Z

@drwells I have rebased the pull request. Please finish your review and merge :-)

No wait, don't merge ! See my comments above. I have to sort out my GitHub stuff before I can add the line you requested :-)

drwells · 2019-08-14T17:46:09Z

@melaniegerault take a look at the branch history. @tamiko cleaned things up very nicely for us.

drwells · 2019-08-14T17:53:01Z

We still need links to the pictures in the function documentation.

tamiko · 2019-08-14T18:18:40Z

@melaniegerault Locally you will have to do something like

$ git remote update  # update all remote refs
$ git checkout eccentric_hyper_shell # make sure you're on the right branch
$ git reset --hard origin/eccentric_hyper_shell # to reset you local branch to the one in your github repo

tamiko · 2019-08-14T18:19:41Z

@drwells @melaniegerault What about we merge and simply fix the link to the pictures in a follow-up PR?

tamiko · 2019-08-14T18:20:30Z

@drwells Alternatively, feel free to simply push a commit with your intended changes to this PR. I am happy to review and merge afterwards.

melaniegerault · 2019-08-14T18:21:57Z

@drwells Alternatively, feel free to simply push a commit with your intended changes to this PR. I am happy to review and merge afterwards.

I have the changes made locally, I'm just not sure how to push them properly. Trying now.

drwells · 2019-08-14T18:26:08Z

@tamiko has a good point: the easiest way forward is to merge this PR as-is and handle images in a followup PR.

drwells · 2019-08-14T18:27:03Z

@melaniegerault Would you mind holding on to your changes for the moment and then putting them on another branch in a moment once this is merged?

melaniegerault · 2019-08-14T18:27:26Z

@tamiko has a good point: the easiest way forward is to merge this PR as-is and handle images in a followup PR.

@tamiko @drwells is it alright now?

melaniegerault · 2019-08-14T18:27:58Z

@melaniegerault Would you mind holding on to your changes for the moment and then putting them on another branch in a moment once this is merged?

I pushed them a few minutes ago

tamiko · 2019-08-14T18:32:40Z

Merged. @drwells @melaniegerault Let's add a reference to the pictures in the documentation of the function in a separate PR.

drwells · 2019-08-14T18:38:51Z

@melaniegerault If you pushed changes recently they were not on this branch, otherwise we would see it somewhere in this PR. Do you know where they ended up?

melaniegerault · 2019-08-14T18:41:47Z

nope, no idea! I was just looking for them. I think I might have erased them with the reset. How do I push them again if this is closed?

drwells · 2019-08-14T18:42:36Z

@melaniegerault We will have to set up a second PR with just those changes.

melaniegerault · 2019-08-14T18:44:39Z

@drwells okay sounds good. Do you want to take the risk of letting me do that? :-)
I would like to, if you don't mind.

drwells · 2019-08-14T18:44:45Z

Its up to you. Its not my intent to torture you with GitHub stuff; I would be happy to apply the patch for you. If you are confident that you can set up another PR with just this change then, by all means, go for it!

melaniegerault · 2019-08-14T18:47:14Z

That's the only way I'll learn! 😁

melaniegerault · 2019-08-14T18:48:29Z

Do you want me to push my low res images to the doc folder or would you prefer to do that yourself?

drwells · 2019-08-14T18:49:47Z

This PR already added the pictures: all we need to add now are the links to them from the documentation.

melaniegerault · 2019-08-14T18:51:48Z

Right, but as you can see, those are the heavy (~200 kb) figures, whereas @tamiko suggested I make lighter ones.

drwells · 2019-08-14T19:30:13Z

I am not too worried about the disk size (we already have a bunch of images that large), but they will look huge in the documentation unless we shrink them first.

drwells added Enhancement Triangulation labels Aug 8, 2019

drwells reviewed Aug 8, 2019

View reviewed changes

tamiko requested changes Aug 8, 2019

View reviewed changes

tamiko added the ready to test label Aug 8, 2019

drwells requested changes Aug 8, 2019

View reviewed changes

melaniegerault force-pushed the eccentric_hyper_shell branch from f1a8438 to 2cae2e8 Compare August 8, 2019 23:40

tamiko approved these changes Aug 8, 2019

View reviewed changes

bangerth reviewed Aug 9, 2019

View reviewed changes

bangerth approved these changes Aug 13, 2019

View reviewed changes

bangerth added the Reviewed and ready to merge label Aug 13, 2019

masterleinad removed the Reviewed and ready to merge label Aug 13, 2019

Melanie Gerault and others added 4 commits August 14, 2019 12:32

Add GridGenerator::eccentric_hyper_shell

86cfa80

Add an eccentric hyper shell geometry that is formed by two spheres with different center points.

Add GridGenerator::eccentric_hyper_shell

0446a91

Add an eccentric hyper shell geometry that is formed by two spheres with different center points.

Update source/grid/grid_generator.cc

dbc76db

Co-Authored-By: David Wells <drwells@email.unc.edu>

Figures for 2D and 3D grids plus edits on grid_generator.cc

4f6439b

tamiko force-pushed the eccentric_hyper_shell branch from d1d7a4b to 4f6439b Compare August 14, 2019 17:40

drwells approved these changes Aug 14, 2019

View reviewed changes

tamiko merged commit 8f86524 into dealii:master Aug 14, 2019

Eccentric hyper shell #8526

Eccentric hyper shell #8526

Conversation

melaniegerault commented Aug 8, 2019

drwells commented Aug 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drwells left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melaniegerault commented Aug 9, 2019

drwells commented Aug 10, 2019

bangerth commented Aug 13, 2019

bangerth commented Aug 13, 2019

melaniegerault commented Aug 13, 2019

melaniegerault commented Aug 13, 2019

drwells commented Aug 13, 2019

tamiko commented Aug 14, 2019

drwells commented Aug 14, 2019

melaniegerault commented Aug 14, 2019

drwells commented Aug 14, 2019

drwells commented Aug 14, 2019

tamiko commented Aug 14, 2019

tamiko commented Aug 14, 2019

tamiko commented Aug 14, 2019

melaniegerault commented Aug 14, 2019

drwells commented Aug 14, 2019

drwells commented Aug 14, 2019

melaniegerault commented Aug 14, 2019

melaniegerault commented Aug 14, 2019

tamiko commented Aug 14, 2019

drwells commented Aug 14, 2019

melaniegerault commented Aug 14, 2019

drwells commented Aug 14, 2019

melaniegerault commented Aug 14, 2019

drwells commented Aug 14, 2019

melaniegerault commented Aug 14, 2019

melaniegerault commented Aug 14, 2019

drwells commented Aug 14, 2019

melaniegerault commented Aug 14, 2019

drwells commented Aug 14, 2019