Allow different boundary temperature plugins on different boundaries #2051

gassmoeller · 2018-01-09T16:51:07Z

This sits on top of #2044 and does the same thing for boundary temperature plugins. Now, one can select different boundary temperature plugins for different boundaries in the same style and input format as for velocities. I still want to make the parsing a bit more intuitive by allowing to only specify plugin names (that are then used for all boundaries) and not only pairs of boundaries and plugins, but this gets a bit complicated due to backwards compatibility to the current system, where a single entry is interpreted as a boundary id (not a plugin name). I need to do some more work on the parsing and documentation before this is finished.

bangerth · 2018-01-09T16:55:42Z

Let me know once you have this rebased after merging #2044.

gassmoeller · 2018-02-15T22:57:55Z

This should be ready for review. I recommend reviewing in 'split' mode, as much of interface.cc has changed. On the upside I was able to remove several deprecated things, so that the new structure looks actually simpler than before. The only complicated part is the Manager::parse_parameters function which figures out which plugins to use on which boundaries.

bangerth · 2018-02-26T01:17:58Z

include/aspect/boundary_temperature/interface.h

-         * The point of this function is to allow complex boundary temperature
-         * models to do an initialization step once at the beginning of each
-         * time step. An example would be a model that needs to call an
-         * external program to compute the temperature change at a boundary.


Any particular reason to remove this comment?

bangerth · 2018-02-26T01:20:31Z

include/aspect/boundary_temperature/interface.h

-        boundary_temperature (const types::boundary_id boundary_indicator,
-                              const Point<dim> &position) const;
+        const std::vector<std_cxx11::shared_ptr<Interface<dim> > > &
+        get_active_boundary_temperature_conditions () const;

        /**
         * Return the minimal temperature of all selected plugins on that
         * part of the boundary on which Dirichlet conditions are posed.
         */


I think this comment is no longer accurate (and hasn't been for a while). That's because it's not that part of the boundary on which Dirichlet conditions are posed, but in fact that subset of Dirichlet boundaries with indicators given by the argument list. While you're here, can you update this? Or maybe leave it for a follow-up, if you prefer given that this likely shows up in a number of places -- in that case, just open an issue.

bangerth · 2018-02-26T01:20:41Z

include/aspect/boundary_temperature/interface.h

-                                    = std::set<types::boundary_id>()) const;
+        double
+        maximal_temperature (const std::set<types::boundary_id> &fixed_boundary_ids
+                             = std::set<types::boundary_id>()) const;


Same for this function.

bangerth · 2018-02-26T01:21:26Z

include/aspect/boundary_temperature/interface.h

+                             = std::set<types::boundary_id>()) const;
+
+        /**
+                * A function that calls the boundary_temperature functions of all the


please fix the indentation of the comment here

also, if you write it as boundary_temperature() (with parens) then doxygen will automatically link to it

bangerth · 2018-02-26T03:48:39Z

source/boundary_temperature/interface.cc

    }



    template <int dim>
    double
-    Manager<dim>::boundary_temperature (const types::boundary_id boundary_indicator,
-                                        const Point<dim> &position) const
+    Manager<dim>::minimal_temperature (const std::set<types::boundary_id> &/*fixed_boundary_ids*/) const


oh, this negates my comment on the documentation of the function -- the function apparently ignores its argument. is this intended? If so, this is not a virtual function, so it's not possible to overload it, and if the only existing implementation ignores the argument, why not drop it altogether?

bangerth · 2018-02-26T03:48:51Z

source/boundary_temperature/interface.cc

@@ -200,13 +135,15 @@ namespace aspect

    template <int dim>
    double
-    Manager<dim>::minimal_temperature (const std::set<types::boundary_id> &fixed_boundary_ids) const
+    Manager<dim>::maximal_temperature (const std::set<types::boundary_id> &/*fixed_boundary_ids*/) const


same here, of course

bangerth · 2018-02-26T03:53:42Z

source/utilities.cc

+        AssertThrow(false,
+                    ExcMessage ("ASPECT only accepts the following operators: "
+                                "add, subtract, minimum and maximum. But your parameter file "
+                                "contains: " + operation + ". Please check your parameter file.") );


just write this as

"contains <" + operation + ">. Please check..."

for consistency with other places.

In some places, we pass the name of the parameter where this happened to similar functions (e..g., to possibly_extend_1_to_N()) so that it's easier to find the location where this happened in the input file. Could we do the same here?

bangerth · 2018-02-26T03:54:54Z

tests/edit_parameters.cc

-        parameters.fixed_temperature_boundary_indicators.erase(bottom);
+        std::set<types::boundary_id> &temperature_boundary_ids =
+          const_cast<std::set<types::boundary_id> &> (simulator_access.get_boundary_temperature_manager().get_fixed_temperature_boundary_indicators());
+        temperature_boundary_ids.erase(bottom);


oh, if this was anywhere other than in a test, I'd be quite unhappy about such a trick :-)

gassmoeller · 2018-02-26T23:51:47Z

I addressed most issues. About your comments:

First comment: I removed the part about the purpose, because it is not happening in this function. The Manager::update function only calls all the update functions of the plugins, and I did not want to duplicate the documentation too much. Would you prefer to keep the paragraph?

I updated the documentation and implementation of the minimal_temperature and maximal_temperature functions to do what they were meant to do. I do not like those functions though, and would vote for deprecating and at some point removing them (maybe replacing them by a representative_surface_temperature and representative_bottom_temperature function in the manager class). I will open a separate issue about that.

I added a parameter to the create_model_operator_list function to indicate the wrong input parameter.

Last comment: I am also not happy about this. Problem is that the test tested functionality that is now not longer easily available. The parameter to change is not longer a member of Parameters, but only Parameters is handed over as writable reference. I am not sure what to do here. It is a pretty exotic case though (removing a boundary condition during the model run).

jdannberg

I don't unerstand all of the technical details, but I guess as @bangerth has already looked at this, this is fine.
@bangerth, as @gassmoeller already addressed your comments, is this okay from your side?

@gassmoeller: You need to add a changelog entry. And I'll add this to the list of cookbooks we need, because I feel this is really not intuitive to understand without an example. Do you think it would be worth it to add a short section to the manual, just to explain the syntax?

jdannberg · 2018-03-29T04:04:59Z

include/aspect/boundary_temperature/interface.h

+         * A map between fixed_temperature_boundary_indicators and one or more
+         * indices into the boundary_temperature_names,
+         * boundary_temperature_objects, and boundary_temperature_operators vectors.
+         * This variable stores which plugins are active for which boundary.


I am a bit confused how this works. Can you extend the comment a bit and explain how this is structured (how do I know what this second index is for; boundary_temperature_name, boundary_temperature_object, or boundary_temperature_operator)?

jdannberg · 2018-03-29T19:25:12Z

source/boundary_temperature/interface.cc

+                           "but not explicitly listed here will end up with no-flux "
+                           "(insulating) boundary conditions."
+                           "\n\n"
+                           "The names of the boundaries listed here can either by "


jdannberg · 2018-03-29T21:15:42Z

source/boundary_temperature/interface.cc

+                           "compute the boundary temperature. If only one plugin is given, "
+                           "only this plugin is used. The same boundary identifier can "
+                           "appear multiple times with different plugin names, in which "
+                           "case all given plugins will be combined to compute the boundary "


Maybe add a sentence to make it more clear that this is the only way to assign more than one plugin to a boundary, because it would not work to write 'boundary identifier : plugin name 1, plugin name 2'. By the way, the only reason that this does not work is that if there is a comma in the list, we do not know if the thing after it is a new boundary indicator or another plugin, right? So what about allowing something like 'boundary identifier : [plugin name 1, plugin name 2]'? I feel that would be much easier to read in the input file than 'boundary identifier : plugin name 1, boundary identifier : plugin name 2', and this way you could check again if any of the indicators is present in the list twice, and let the user know, so it would be less error prone.

After thinking about this for a while, I guess the most reasonable approach I see would be to change this format to allow something like:

set Fixed temperature boundary indicators = top : box & function & constant, left : box & initial temperature, bottom

If there is no colon, all plugins are applied (as it is right now, so it's backwards compatible). I guess that means duplicating the list of plugins in the 'List of model names' parameter, but I don't really see any other way to do this, and it would only come up in complex models.

So I guess the only other question we have to think about here is: Do we ever want to allow having the same kind of plugin, but with different parameters, for different boundaries (like 2 different functions)? Because I don't really see how we would put that into this framework, so we would essentially say that this is someting we do not want to change in the near future.

I haven't looked at this since my last review, but just about the format: instead of & we've generally used ; as a top-level separator between entries I believe.

jdannberg · 2018-03-29T21:27:31Z

source/boundary_temperature/interface.cc

+                           "on which the temperature is fixed and described by the "
+                           "boundary temperature objects selected in the "
+                           "<Boundary temperature model/List of model names> parameter "
+                           "of this input file. All boundary indicators used by the geometry "


One question: With this new format, is there still a reason to even have the section Boundary temperature model/List of model names other than backwards compatibility? Or in other words, should we also allow users to put in boundary temperature plugins here that they have not selected in the List of model names? This way, we could deprecate the other parameter and remove it at some point, and we would end up with the same format for the temperature and the velocity boundary conditions.

Update: I see, you need it to apply the operators. So do we have plans for the future to allow several plugins for the velocity boundary conditions too? Because then I guess it would make sense to think about a format that would work for all three (temperature, composition, velocity).

jdannberg · 2018-03-29T21:40:08Z

source/boundary_temperature/interface.cc

+      prm.leave_subsection();
+
+      // Now create and initialize all of the requested models. We can only do this here at the end,
+      // because some models might ask, which boundary they are responsible for.


remove comma

jdannberg · 2018-03-29T21:41:54Z

source/initial_temperature/interface.cc

@@ -123,7 +123,8 @@ namespace aspect
          Utilities::possibly_extend_from_1_to_N (Utilities::split_string_list(prm.get("List of model operators")),
                                                  model_names.size(),
                                                  "List of model operators");
-        model_operators = Utilities::create_model_operator_list(model_operator_names);
+        model_operators = Utilities::create_model_operator_list(model_operator_names,
+                                                                "Boundary composition model/List of model operators");


This should be initial temperature, not boundary composition, right? And maybe add a test for that if we haven't caught this with any of the existing tests?

jdannberg · 2018-03-29T22:18:11Z

source/utilities.cc

+            {
+              AssertThrow(false,
+                          ExcMessage ("ASPECT only accepts the following operators: "
+                                      "add, subtract, minimum and maximum. But your parameter file "


Is there a way to get the operators we allow from the Operator object instead of spelling them out by hand? This would allow adding operators in the future without having to change the error message by hand (which we might forget).

jdannberg · 2018-03-29T22:36:27Z

source/boundary_temperature/interface.cc

@@ -272,17 +275,6 @@ namespace aspect
                          "will be used to append the listed temperature models onto "
                          "the previous models. If only one operator is given, "
                          "the same operator is applied to all models.");


I know this doesn't have anything to do with your pull request, but can you please add a sentence here (and in other places where we declare these operators), that this list of operators has to be exactly as long as the list of plugins (if it doesn't have just one entry), and that the order has to be the same as in the 'List of model names' because each operator will be applied to a plugin according to the order they're given in.

gassmoeller · 2018-04-10T20:25:54Z

Same as #2099, lets postpone this after 2.0.

bangerth · 2019-05-23T02:28:48Z

@gassmoeller: What's the status here? Can you rebase and should I then take a look?

gassmoeller force-pushed the extend_boundary_temperature_manager branch from 0864574 to c907ea7 Compare January 10, 2018 00:00

gassmoeller added this to the 2.0 milestone Jan 30, 2018

gassmoeller mentioned this pull request Jan 30, 2018

Allow for specifying separate composition and temperature descriptions for each boundary #1560

Open

gassmoeller force-pushed the extend_boundary_temperature_manager branch 5 times, most recently from 4569c47 to 66eef39 Compare February 15, 2018 22:55

gassmoeller changed the title ~~[WIP] Allow different boundary temperature plugins on different boundaries~~ Allow different boundary temperature plugins on different boundaries Feb 15, 2018

This was referenced Feb 16, 2018

Allow different boundary composition plugins on different boundaries #2099

Open

Remove model settings subsection #2105

Closed

bangerth reviewed Feb 26, 2018

View reviewed changes

gassmoeller force-pushed the extend_boundary_temperature_manager branch from ce75622 to 1c06bc1 Compare February 26, 2018 23:52

gassmoeller force-pushed the extend_boundary_temperature_manager branch from 1c06bc1 to 8771bf9 Compare March 7, 2018 03:21

Allow different temperature plugins at boundaries

0812821

gassmoeller force-pushed the extend_boundary_temperature_manager branch from 8771bf9 to 2da2617 Compare March 7, 2018 03:23

Address comments

3af7d82

gassmoeller force-pushed the extend_boundary_temperature_manager branch from 2da2617 to 3af7d82 Compare March 7, 2018 11:05

jdannberg requested changes Mar 29, 2018

View reviewed changes

gassmoeller mentioned this pull request Apr 10, 2018

Remove model settings subsection #2157

Merged

gassmoeller removed this from the 2.0 milestone Apr 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow different boundary temperature plugins on different boundaries #2051

Allow different boundary temperature plugins on different boundaries #2051

gassmoeller commented Jan 9, 2018

bangerth commented Jan 9, 2018

gassmoeller commented Feb 15, 2018

bangerth Feb 26, 2018

bangerth Feb 26, 2018

bangerth Feb 26, 2018

bangerth Feb 26, 2018

bangerth Feb 26, 2018

bangerth Feb 26, 2018

bangerth Feb 26, 2018

bangerth Feb 26, 2018

bangerth Feb 26, 2018

gassmoeller commented Feb 26, 2018

jdannberg left a comment

jdannberg Mar 29, 2018

jdannberg Mar 29, 2018

jdannberg Mar 29, 2018

jdannberg Apr 3, 2018

bangerth Apr 4, 2018

jdannberg Mar 29, 2018

jdannberg Mar 29, 2018

jdannberg Mar 29, 2018

jdannberg Mar 29, 2018

jdannberg Mar 29, 2018

jdannberg Mar 29, 2018

gassmoeller commented Apr 10, 2018

bangerth commented May 23, 2019

Allow different boundary temperature plugins on different boundaries #2051

Are you sure you want to change the base?

Allow different boundary temperature plugins on different boundaries #2051

Conversation

gassmoeller commented Jan 9, 2018

bangerth commented Jan 9, 2018

gassmoeller commented Feb 15, 2018

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

gassmoeller commented Feb 26, 2018

jdannberg 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gassmoeller commented Apr 10, 2018

bangerth commented May 23, 2019