Initial topography plugin prm polygon #1099

MFraters · 2016-07-02T00:49:40Z

This adds an initial topography plugin which defines the initial topography by defining 2d polygons on the surface and setting it to the desired elevation. After this I will update the ellipsoidal chunk and make a test out of this. Or should I do this within this pull request?

MFraters · 2016-07-02T01:04:27Z

I added a function to utilities which computes if a point falls within a 2d polygon. I added it there because I noticed that it is a function I use a lot in my plugins, especially for initial conditions. So I thought it might be useful for other people as well.

bangerth · 2016-07-05T13:35:59Z

include/aspect/geometry_model/initial_topography_model/prm_polygon.h

+        /**
+         * The polygons and their points are stored in this vector.
+         */
+        std::vector<std::vector<std::vector<double> > > point_lists;


Since you know the size of the inner vector, can you change the inner vector to Point<dim> or Point<dim-1> or whatever is the appropriate type?

bangerth · 2016-07-05T13:55:35Z

Nice addition. I think it would be nice to have a test as well.

MFraters · 2016-07-20T11:10:30Z

I hope this addresses all comments :)

bangerth · 2016-07-24T04:00:48Z

source/geometry_model/initial_topography_model/prm_polygon.cc

+            {
+              value = topography_values[i];
+            }
+        }


The previous comments on this code have been erased during renaming of the function, but to rehash: I said that this is inefficient because it has to go through all of the point lists, and then takes the value of the last polygon that matches. You said that this is the intended behavior.

I think I don't understand your use case where you have multiple polygons, and more than one of them actually has the given point inside. If that's the case, why take the last polygon's value, rather than the first? Taking the first has the advantage that you can just exit the function at this point, rather than having to search through all following polygons.

You are right it is inefficient. My point was mainly that the last one should be chosen. It is more a conceptual thing for me, that you go through the list and 'paint over' in every polygon. I have made it more efficient now, in the sense that I turned around the for loop and put the return inside the if statement.

Although it currently has my preference that the last one is the final one, but I do not feel very strongly about it. So if you really prefer it to be the other way around I am fine with it.

bangerth · 2016-07-25T16:43:27Z

source/geometry_model/initial_topography_model/prm_polygon.cc

+                              "The next topography height > the next point list describing a polygon.\" "
+                              "The format for the point list describing the polygon is \"x1,y1;x2,y2\". "
+                              "For example for two triangular areas of 100 and -100 meters high set: "
+                              "'100 > 0,0;5,5;0,10 & -100 > 10,10;10,15;20,15'. "


Thinking about it, this won't render correctly in latex. I think you need to write $>$ or maybe better, \textgreater.

bangerth · 2016-07-25T16:45:58Z

I was going to merge, but then I saw the issue with > in latex. Then I thought I'd also point out the issues with the copyright dates. Can you fix these, and squash everything into one commit?

Thinking about it, you probably also want to add an entry to the changes.h file, so you get credit where credit is due.

Otherwise ready to merge.

bangerth · 2016-07-25T16:46:02Z

/run-tests

bangerth · 2016-07-25T17:08:23Z

source/geometry_model/initial_topography_model/prm_polygon.cc

+        {
+          if (aspect::Utilities::polygon_contains_point<dim>(point_lists[i-1],p1))
+            {
+              return topography_values[i-1];


This order of the loop works for me. You may want to add a comment explaining why this is the order in which you go through the loop this way.

MFraters · 2016-07-26T15:08:18Z

I must say that it currently still is not usable, because no geometry model has implemented the initial topography yet. But that will be the next step :)

About the squashing, I normally squash (fixup actually) all changes in one source and include pair in one commit, and each test in one commit. Is this what you meant, or do you want everything into one commit which contains everything?

Furthermore, I noticed that before adding the last set of changes (after which the tester ran again), the tester failed because three melt tests where taking to long. For now it is fine, but maybe the allowed time per test should be extended a bit, or the tests should be simplified a bit in the future?

bangerth · 2016-07-26T23:15:15Z

Yes, one for code changes, one for changelog, one for tests is totally fine. That's how I often do it as well.

Regarding tester time, that's @tjhei 's call. But I haven't seen this problem before, so I suspect that it doesn't happen very often and in fact only happens if something else is happening in the background.

bangerth · 2016-07-26T23:16:22Z

doc/modules/changes.h

@@ -6,6 +6,12 @@
 *
 * <ol>
 *
+ * <li> New: There is now an initial topography plugin which reads 
+ * from the prm file polygon definitons and set the initial topography 


-> definitions

bangerth · 2016-07-26T23:19:51Z

I was going to merge, because the structure with the 4 commits is also just fine -- in fact, it's a nice way of doing it this way because the function that tests whether a point is inside a polygon is a self-contained addition that can certainly be in its own commit.

But I saw that you have two typos in the changelog. Can you still fix them? (Logically, btw, the second commit with polygon_contains_point should come before the first commit because currently the first requires the second -- in other words, you can't put yourself onto the first commit and compile ASPECT -- you will get a compiler error. That's really not an important issue, but it's also really easy to fix with git rebase -i by just swapping two lines.)

MFraters · 2016-07-27T11:33:19Z

Thanks! Your are right, that order makes more sense.

I also noticed that there where still two comments in prm_polygon.h incorrect, so I fixed them (hopefully didn't introduce more spelling mistakes :P )

MFraters · 2016-07-27T15:35:13Z

Please do not merge yet, I found a possible mistake somewhere. I want to check it first.

bangerth · 2016-07-27T15:47:59Z

OK. Let us know when you have it fixed :-)

MFraters · 2016-07-27T23:50:27Z

I found that the name was not of the subsection was not consistent with the interface, and that is now fixed.

At the same time I was making the implementation of this in the ellipsoidal chunk geometry model, and I found some weird behaviour. But I am now quite sure that doesn't have to do with this part of the code, but with what happens when you use a very course grid with a very large topography. But that is something we might discuss in that pull request.

In short, if the tester is fine with this, I think it is ready to merge :)

MFraters force-pushed the topo_prm_polygon branch 2 times, most recently from 45d68e2 to 0eba453 Compare July 2, 2016 01:02

bangerth reviewed Jul 5, 2016
View reviewed changes

MFraters force-pushed the topo_prm_polygon branch from 0eba453 to c693ef1 Compare July 20, 2016 11:01

bangerth reviewed Jul 24, 2016
View reviewed changes

MFraters force-pushed the topo_prm_polygon branch from c693ef1 to 427704f Compare July 25, 2016 14:03

bangerth reviewed Jul 25, 2016
View reviewed changes

MFraters force-pushed the topo_prm_polygon branch from 427704f to 80f6fa9 Compare July 26, 2016 08:22

bangerth reviewed Jul 26, 2016
View reviewed changes

Add in_2d_polygon function to utilities.

e30ab3a

MFraters force-pushed the topo_prm_polygon branch 2 times, most recently from c61c37a to b2736bd Compare July 27, 2016 09:14

MFraters added 3 commits July 28, 2016 01:42

Initial topograpy prm polygon.

6dee401

Add specific test for prm polygon plugin.

41a596a

Add prm_polygon to changes.h.

6663c29

MFraters force-pushed the topo_prm_polygon branch from b2736bd to 6663c29 Compare July 27, 2016 23:42

bangerth merged commit 1532b0f into geodynamics:master Jul 31, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial topography plugin prm polygon #1099

Initial topography plugin prm polygon #1099

MFraters commented Jul 2, 2016

MFraters commented Jul 2, 2016

bangerth Jul 5, 2016

bangerth commented Jul 5, 2016

MFraters commented Jul 20, 2016

bangerth Jul 24, 2016

MFraters Jul 25, 2016 •

edited

bangerth Jul 25, 2016

bangerth commented Jul 25, 2016

bangerth commented Jul 25, 2016

bangerth Jul 25, 2016

MFraters commented Jul 26, 2016

bangerth commented Jul 26, 2016

bangerth Jul 26, 2016

bangerth commented Jul 26, 2016

MFraters commented Jul 27, 2016

MFraters commented Jul 27, 2016

bangerth commented Jul 27, 2016

MFraters commented Jul 27, 2016

Initial topography plugin prm polygon #1099

Initial topography plugin prm polygon #1099

Conversation

MFraters commented Jul 2, 2016

MFraters commented Jul 2, 2016

bangerth Jul 5, 2016

Choose a reason for hiding this comment

bangerth commented Jul 5, 2016

MFraters commented Jul 20, 2016

bangerth Jul 24, 2016

Choose a reason for hiding this comment

MFraters Jul 25, 2016 • edited

Choose a reason for hiding this comment

bangerth Jul 25, 2016

Choose a reason for hiding this comment

bangerth commented Jul 25, 2016

bangerth commented Jul 25, 2016

bangerth Jul 25, 2016

Choose a reason for hiding this comment

MFraters commented Jul 26, 2016

bangerth commented Jul 26, 2016

bangerth Jul 26, 2016

Choose a reason for hiding this comment

bangerth commented Jul 26, 2016

MFraters commented Jul 27, 2016

MFraters commented Jul 27, 2016

bangerth commented Jul 27, 2016

MFraters commented Jul 27, 2016

MFraters Jul 25, 2016 •

edited