New function for boundary composition #1703

mbweller · 2017-05-16T03:29:40Z

Function added to boundary composition to allow for variable compositions to placed with user defined coordinate systems. Boundary composition is now consistent with Boundary: Temperature, Velocity, and traction.

gassmoeller

Very nice, please address the comments and add or modify a test to use this functionality

gassmoeller · 2017-05-16T14:52:16Z

include/aspect/boundary_composition/function.h

+      public:
+        /**
+         * Constructor.
+         */


Remove this comment there is no constructor.

gassmoeller · 2017-05-16T14:52:47Z

source/boundary_composition/function.cc

+    //Function<dim>::Function ()
+    //  :
+    //  boundary_composition_function (1)
+    //{}


remove the commented lines

gassmoeller · 2017-05-16T14:53:22Z

include/aspect/boundary_composition/function.h

+        /**
+         * A function object representing the composition.
+         */
+        Functions::ParsedFunction<dim> boundary_composition_function;


this variable is not used in the .cc file, you only use function. Please remove the variable and the comment.

I think this is needed for boundary_composition_function in the .cc file... (other comment below)

gassmoeller · 2017-05-16T14:54:33Z

source/boundary_composition/function.cc

+      if (this->convert_output_to_years())
+        boundary_composition_function.set_time (this->get_time() / year_in_seconds);
+      else
+        boundary_composition_function.set_time (this->get_time());


you need to call this for the function object that you use in the boundary_composition function, not this one one (which is not needed).

gassmoeller · 2017-05-16T14:56:53Z

source/boundary_composition/function.cc

+      // yet. so get it from the parameter file directly.
+      prm.enter_subsection ("Compositional fields");
+      const unsigned int n_compositional_fields = prm.get_integer ("Number of fields");
+      prm.leave_subsection ();


Replace this the same way you did in your other pull request. Also remove the outdated comment above.

This one may not be quite so outdated. It fails if not in place.

gassmoeller · 2017-05-16T14:57:52Z

Also add an entry to doc/modules/changes/

mbweller · 2017-05-16T19:11:19Z

Updated

mbweller · 2017-05-17T00:25:57Z

Updated to not break tests

bangerth · 2017-05-17T02:10:02Z

source/boundary_composition/function.cc

+    double
+    Function<dim>::
+    boundary_composition (const types::boundary_id /*boundary_indicator*/,
+                          const Point<dim> &position, const unsigned int n_comp) const


please break arguments onto separate lines

please rename the last argument (also in the declaration, if applicable). The original declataion in the base class looks like this:

virtual double boundary_composition (const types::boundary_id boundary_indicator, const Point<dim> &position, const unsigned int compositional_field) const;

bangerth · 2017-05-17T02:12:10Z

source/boundary_composition/function.cc

+                             "parameter is non-zero, which is interpreted to "
+                             "be the depth of the point.");
+
+          Functions::ParsedFunction<dim>::declare_parameters (prm, 1);


this is wrong -- it will only work for a single compositional field. but you want this to work also for multiple fields.

the function.cc in the initial_composition has the same line, not sure why it works. When I use this ->... in Functions::ParsedFunction::declare_parameters (prm, 1); I receive 'this is unavailable for static member functions'

bangerth · 2017-05-17T02:12:38Z

source/boundary_composition/function.cc

+
+          try
+            {
+              function.reset (new Functions::ParsedFunction<dim>(this->n_compositional_fields()));


...because here you are creating a function of multiple arguments.

bangerth · 2017-05-17T02:13:21Z

source/boundary_composition/function.cc

+                                               "post-processing, this boundary composition model "
+                                               "must therefore be told what the minimal and "
+                                               "maximal values on the boundary are. This is done "
+                                               "using parameters set in section ``Boundary composition model/Initial composition''."


break long line

bangerth · 2017-05-17T02:14:04Z

source/initial_composition/function.cc

@@ -26,9 +26,6 @@ namespace aspect
 {
  namespace InitialComposition
  {
-//    template <int dim>
-//    Function<dim>::Function ()
-//    {}


good catch -- this could have been a separate pull request, by the way. the change is conceptually independent, and it is easier to review independent things independently

bangerth · 2017-05-17T02:14:48Z

tests/boundary_composition-passive_cartesian.prm

+  subsection Function
+    set Variable names      = x, y
+    set Coordinate system   = cartesian
+    set Function expression = if(y<0.2, 1,  0) ; if(y>0.5, 1, 0)


does this work? I thought you declared parameters for a scalar function?

I have no idea why it works, but it seems to do so.

bangerth · 2017-05-17T02:15:51Z

tests/boundary_composition-passive_sphere.prm

+
+subsection Mesh refinement
+  set Initial adaptive refinement        = 0
+  set Initial global refinement          = 5


that's a lot of refinement, and consequently a lot of data that goes with the test. can you reduce this number? To verify the correctness of your class, you don't actually need a very fine mesh :-)

mbweller · 2017-05-17T03:16:58Z

Some changes made. Some not...

mbweller · 2017-05-17T03:23:27Z

Fixed with astyle

gassmoeller · 2017-05-17T03:23:42Z

Did you talk to @bangerth about the function arguments?

gassmoeller · 2017-05-17T03:23:48Z

Also please run astyle

bangerth · 2017-05-17T04:04:48Z

One tester timed out. Let us postpone this till we're all home again. If something works but we don't understand why or how, then that would suggest that we need to work a bit harder to convince ourselves that this is correct.

gassmoeller · 2017-05-23T03:03:48Z

@mbweller please update master once and rebase this PR. We should have fixed the tester issues, and will then see which test causes the errors.

gassmoeller · 2017-05-23T03:03:59Z

/run-tests

gassmoeller · 2017-07-07T16:48:02Z

Hi Matt,
will you have time to address the comments within the next weeks? I am currently going through our issues and pull requests to see what can be finished, and this seems like a low-hanging fruit. If you do not have time that is not a problem, I can take over and finish the remaining bits (you keep the credit for everything that is already there).

gassmoeller · 2017-10-02T17:48:11Z

Superseded and merged by #1943. Thanks @mbweller for the idea and initial implementation.

gassmoeller reviewed May 16, 2017

View reviewed changes

mbweller force-pushed the boundary_composition_function branch from 80f9c14 to a016bc3 Compare May 16, 2017 19:11

mbweller force-pushed the boundary_composition_function branch from a016bc3 to eb38b04 Compare May 17, 2017 00:25

bangerth requested changes May 17, 2017

View reviewed changes

mbweller force-pushed the boundary_composition_function branch from eb38b04 to 457ec7c Compare May 17, 2017 03:13

New function for boundary composition

b6c063e

mbweller force-pushed the boundary_composition_function branch from 457ec7c to b6c063e Compare May 17, 2017 03:23

gassmoeller mentioned this pull request May 22, 2017

add clever abort handling to avoid MPI deadlock #1753

Merged

gassmoeller mentioned this pull request Oct 1, 2017

Boundary composition function #1943

Merged

gassmoeller closed this Oct 2, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New function for boundary composition #1703

New function for boundary composition #1703

mbweller commented May 16, 2017

gassmoeller left a comment

gassmoeller May 16, 2017

gassmoeller May 16, 2017

gassmoeller May 16, 2017

mbweller May 16, 2017

gassmoeller May 16, 2017

gassmoeller May 16, 2017

mbweller May 16, 2017

gassmoeller commented May 16, 2017

mbweller commented May 16, 2017

mbweller commented May 17, 2017

bangerth May 17, 2017

bangerth May 17, 2017

bangerth May 17, 2017

mbweller May 17, 2017

bangerth May 17, 2017

bangerth May 17, 2017

bangerth May 17, 2017

bangerth May 17, 2017

mbweller May 17, 2017

bangerth May 17, 2017

mbweller commented May 17, 2017

mbweller commented May 17, 2017

gassmoeller commented May 17, 2017

gassmoeller commented May 17, 2017

bangerth commented May 17, 2017

gassmoeller commented May 23, 2017

gassmoeller commented May 23, 2017

gassmoeller commented Jul 7, 2017

gassmoeller commented Oct 2, 2017

New function for boundary composition #1703

New function for boundary composition #1703

Conversation

mbweller commented May 16, 2017

gassmoeller 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

gassmoeller commented May 16, 2017

mbweller commented May 16, 2017

mbweller commented May 17, 2017

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

Choose a reason for hiding this comment

mbweller commented May 17, 2017

mbweller commented May 17, 2017

gassmoeller commented May 17, 2017

gassmoeller commented May 17, 2017

bangerth commented May 17, 2017

gassmoeller commented May 23, 2017

gassmoeller commented May 23, 2017

gassmoeller commented Jul 7, 2017

gassmoeller commented Oct 2, 2017