Compositing MaterialModel #1602

class4kayaker · 2017-05-12T22:56:12Z

Work at generating the model suggested in #1529 (also #691). I would appreciate any suggestions on how to better arrange the internals, and what might be a good test case.

tjhei · 2017-05-13T00:12:47Z

@jdannberg ?
/run-tests

tjhei · 2017-05-13T00:43:35Z

source/material_model/compositing.cc

+      if (model_property_map.at(Property::entropy_derivative_temperature) == model_index)
+        out.entropy_derivative_temperature = evaluated.entropy_derivative_temperature;
+      if (model_property_map.at(Property::reaction_terms) == model_index)
+        out.reaction_terms == evaluated.reaction_terms;


this looks incorrect.

jdannberg

It would also be great if you could add at least one test for your new material model (one idea would be just copying an existing test and getting all properties from the original model, and then the test results should stay exactly the same). That would also show how the input parameters for the new models would look like.

And thanks for doing this!

jdannberg · 2017-05-13T00:46:16Z

source/material_model/compositing.cc

+    ASPECT_REGISTER_MATERIAL_MODEL(Compositing,
+                                   "compositing",
+                                   "The ``compositing'' Material model selects material model properties from a "
+                                   "given set of other material models."


It would be great if you could explain a bit more what this material model does and how exactly it achieves this (and what all of the input options are).

jdannberg · 2017-05-13T00:56:01Z

include/aspect/material_model/compositing.h

+         * Obtain the output properties from the component material models
+         */
+        void
+        composite(const unsigned int model_index,


I feel this name does not describe very well what the function does. Maybe get_properties_from_base_models?
And can you document the arguments of the function?

jdannberg · 2017-05-13T00:58:09Z

include/aspect/material_model/compositing.h

+
+        /**
+         * Map specifying which components a material model is responsible for
+         */


Maybe you can make this a bit more specific.

jdannberg · 2017-05-13T01:02:40Z

source/material_model/compositing.cc

+          prm.declare_entry("Models", "",
+                            Patterns::Map(Patterns::Selection("viscosity|density|thermal expansion coefficient|specific heat|compressibility|entropy derivative pressure|entropy derivative temperature|reaction terms"),
+                                          Patterns::Selection(MaterialModel::get_valid_model_names_pattern<dim>())),
+                            "The material property and material model associations used for this composite material model");


In the Depth dependent model we call this parameter Base models, so we should make that consistent. And can you also document how exactly the input format has to look like?

This has been altered to be a list of properties (generated from a static const map listing names and the corresponding enum) which should be much more readable and robust.

jdannberg · 2017-05-13T01:18:31Z

source/material_model/compositing.cc

+            = Utilities::split_string_list
+              (prm.get("Models"));
+          for (std::vector<std::string>::const_iterator p = x_models.begin();
+               p != x_models.end(); ++p)


In general, it would be great if the variable names could be more descriptive than x_models and p.

jdannberg · 2017-05-13T01:19:14Z

source/material_model/compositing.cc

+              AssertThrow(split_parts[1] != "compositing",
+                          ExcMessage("You may not use ``compositing'' as the base model for "
+                                     "a compositing material model."));
+              unsigned int model_ind;


Same here, just call it model index.

jdannberg · 2017-05-13T01:32:53Z

source/material_model/compositing.cc

+                               typename Interface<dim>::MaterialModelOutputs &out) const
+    {
+      typename Interface<dim>::MaterialModelOutputs evaluated(out.viscosities.size(),
+                                                              this->introspection().n_compositional_fields);


Maybe call those base_outputs instead of evaluated?

jdannberg · 2017-05-13T01:35:36Z

source/material_model/compositing.cc

+
+              model_property_map[prop] = model_ind;
+            }
+          models.resize(model_names.size());


It would be great if you could write a comment about what the code above does.

class4kayaker · 2017-05-13T02:22:28Z

I have added a test that is failing despite copying all of the input and output from simple_incompressible. I will work on identifying the source of the issue, though my guess is that it originates from how I am copying the outputs.

gassmoeller · 2017-05-13T21:38:04Z

Take a look at the diff file generated by the tester. All of these changes are expected (a different output directory, and no basic statistics output anymore, because that one is only computed if you use the simple material model)

gassmoeller

A lot of comments, but only one is maybe a bug. I like the concept, and have mostly style comments. Thanks for working on this 👍

gassmoeller · 2017-05-26T15:05:30Z

include/aspect/material_model/compositing.h

+  <http://www.gnu.org/licenses/>.
+*/
+
+#ifndef _aspect_material_model_averaging_h


please update the include guard, this looks like copy-pasted from averaging? or did you mean compositing?

gassmoeller · 2017-05-26T15:05:50Z

include/aspect/material_model/compositing.h

+{
+  namespace MaterialModel
+  {
+    using namespace dealii;


do you need this? If not remove it.

gassmoeller · 2017-05-26T15:10:47Z

include/aspect/material_model/compositing.h

+         *
+         * @param model_index Internal index for pointer to the material model evaluated
+         * @param base_output Properties generated by the material model specified
+         * @out MaterialModelOutputs to be used.


From the description I do not quite follow what this function does. Please extend the first sentence a bit. E.g. which material model? Supposedly not this object, but one of the models responsible for one of the properties?

After looking at the parameters closer, I think I got its purpose. Probably it copies all properties that are requested from one particular (sub)-model into the final MaterialModelOutputs object?

gassmoeller · 2017-05-26T15:11:59Z

source/material_model/compositing.cc

+  <http://www.gnu.org/licenses/>.
+*/
+
+#include <deal.II/base/std_cxx11/array.h>


I do not think you need this

gassmoeller · 2017-05-26T15:12:18Z

source/material_model/compositing.cc

+#include <utility>
+#include <limits>
+
+using namespace dealii;


do you need this?

gassmoeller · 2017-05-26T15:40:44Z

source/material_model/compositing.cc

+
+          // create the model and initialize their SimulatorAccess base
+          // class; it will get a chance to read its parameters below after we
+          // leave the current section


I see why you can not do the parsing here, but can you just move these lines below out of the subsection? It would be easier to understand if there were only one loop over the model names.

gassmoeller · 2017-05-26T15:41:02Z

source/material_model/compositing.cc

+          this -> model_dependence.density |= models[i]->get_model_dependence().density;
+          this -> model_dependence.compressibility |= models[i]->get_model_dependence().compressibility;
+          this -> model_dependence.specific_heat |= models[i]->get_model_dependence().specific_heat;
+          this -> model_dependence.thermal_conductivity |= models[i]->get_model_dependence().thermal_conductivity;


please remove the spaces around the arrows in the lines above

gassmoeller · 2017-05-26T15:41:14Z

source/material_model/compositing.cc

+    Compositing<dim>::
+    is_compressible () const
+    {
+      unsigned int ind = model_property_map.at(Property::compressibility);


make it const

gassmoeller · 2017-05-26T15:41:24Z

source/material_model/compositing.cc

+    Compositing<dim>::
+    reference_viscosity() const
+    {
+      unsigned int ind = model_property_map.at(Property::viscosity);


gassmoeller · 2017-05-26T15:42:58Z

tests/simple_composite.prm

+# Duplicate of the simple incompressible model with using the
+# composite model instead
+
+# This is a relatively realistic compressible model setup.


Please check the comments at the beginning of the file. Make it consistent.

class4kayaker · 2017-05-26T19:10:00Z

Changes made.

gassmoeller

One more questionable condition, and one more style comment. Otherwise good to go.

gassmoeller · 2017-05-26T19:16:56Z

source/material_model/compositing.cc

-                     ExcMessage ("The value <" + s + "> for a material "
-                                 "property is not one of the valid values."));
-      return Property::viscosity;
+      AssertThrow (Property::property_map.count(s) == 0,


Should it not be Property::property_map.count(s) > 0?

gassmoeller · 2017-05-26T19:30:20Z

source/material_model/compositing.cc

      for (unsigned int i=0; i<model_names.size(); ++i)
        {
+          models.at(i).reset(create_material_model<dim>(model_names[i]));


One more nitpick (I just mention it because I have the above comment anyway): Now you resize models above (probably for speed?), but then use at here instead of [] further down, probably just copied from the earlier loop? Two comments about that:

Please move the resize command in front of this loop, where models is used, so that related commands are close together.

Use consistently at or [] do not mix between them. In this case [] is totally safe, because i will never exceed the bounds of the vector. In general at is significantly slower (see e.g. https://stackoverflow.com/questions/9376049/vectorat-vs-vectoroperator), although of course in this case with just a few entries this does not matter at all.

gassmoeller · 2017-05-29T20:54:21Z

Thanks!

Create composite material model

b6d822d

class4kayaker force-pushed the compositing branch from 970d399 to b6d822d Compare May 12, 2017 22:56

class4kayaker changed the title ~~Compositing~~ Compositing MaterialModel May 12, 2017

tjhei reviewed May 13, 2017

View reviewed changes

class4kayaker added 2 commits May 12, 2017 18:38

Adjust for simpler/more robust declaration of parameters

ea6db00

Add failing test

6c50b94

jdannberg requested changes May 13, 2017

View reviewed changes

class4kayaker added 6 commits May 12, 2017 18:46

Change to use all values from single model as simple test

26cbbf1

Adjust names/documentation

31d7bca

Change to duplicate simple_incompressible

dc466bf

Fix small bugs

0865a13

Check more possible error sources

f49e1e1

Adjust information displayed

7fce6ce

class4kayaker added 2 commits May 13, 2017 16:33

Change output to match tester as all expected

407daeb

Adjust documentation in simple composite tests

16829aa

gassmoeller reviewed May 26, 2017

View reviewed changes

Correct suggestions

f5d4b3e

class4kayaker force-pushed the compositing branch from 8d8c791 to f5d4b3e Compare May 26, 2017 17:19

gassmoeller reviewed May 26, 2017

View reviewed changes

class4kayaker added 2 commits May 26, 2017 12:53

Fix comments

4dfd451

Add changelog entry

fcc426c

gassmoeller merged commit 7840b34 into geodynamics:master May 29, 2017

gassmoeller mentioned this pull request May 30, 2017

Fix test #1792

Merged

bangerth mentioned this pull request May 30, 2017

Clean up compositing material model. #1793

Merged

class4kayaker deleted the compositing branch May 30, 2017 21:01

bangerth mentioned this pull request Jun 16, 2017

Remove a function that is no longer used. #1815

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compositing MaterialModel #1602

Compositing MaterialModel #1602

class4kayaker commented May 12, 2017

tjhei commented May 13, 2017

tjhei May 13, 2017

jdannberg left a comment

jdannberg May 13, 2017

jdannberg May 13, 2017

jdannberg May 13, 2017

jdannberg May 13, 2017

class4kayaker May 13, 2017

jdannberg May 13, 2017

jdannberg May 13, 2017

jdannberg May 13, 2017

jdannberg May 13, 2017

class4kayaker commented May 13, 2017

gassmoeller commented May 13, 2017

gassmoeller left a comment

gassmoeller May 26, 2017

gassmoeller May 26, 2017

gassmoeller May 26, 2017

gassmoeller May 26, 2017

gassmoeller May 26, 2017

gassmoeller May 26, 2017

gassmoeller May 26, 2017

gassmoeller May 26, 2017

gassmoeller May 26, 2017

gassmoeller May 26, 2017

class4kayaker commented May 26, 2017

gassmoeller left a comment

gassmoeller May 26, 2017

gassmoeller May 26, 2017

gassmoeller commented May 29, 2017

Compositing MaterialModel #1602

Compositing MaterialModel #1602

Conversation

class4kayaker commented May 12, 2017

tjhei commented May 13, 2017

Choose a reason for hiding this comment

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

class4kayaker commented May 13, 2017

gassmoeller commented May 13, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

class4kayaker commented May 26, 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

gassmoeller commented May 29, 2017