Extend parse function to allow multiple values per key #3267

gassmoeller · 2019-10-18T22:54:46Z

This PR extends Utilities::parse_map_to_double_array to allow multiple entries per key using the syntax: key1: value1|value2|value3, key2: value1, key3: value1|value2. We would like to use this syntax to make progress with defining phase transitions for multiple compositional fields (like #2656). The unit tests should cover all cases, so I can make this as a separate PR without actual implementations using it (it will be used by several rheologies, equations of state and material models).

I have tested my new feature locally to ensure it is correct.
I have created a testcase for the new feature/benchmark in the tests/ directory.

gassmoeller · 2019-10-18T22:55:17Z

@jdannberg, @naliboff, @mibillen can one of you take a look if this is what we talked about?

naliboff

Thanks! Only caught one potential typo, but at first read through looks good!

source/utilities.cc

gassmoeller · 2019-10-19T00:11:30Z

Thanks for the quick review. I have fixed the comment and added one more functionality we will need. That is to return the actual substructure of the returned vector. As discussed we need this to ensure that all entries actually have the same number of phases for each individual composition (e.g. to distinguish a:1|2,b:3 from a:1,b:2|3).

jdannberg

Very nice! I really like the structure, and the parsing is not that much more complicated.

jdannberg · 2019-10-19T00:13:34Z

include/aspect/utilities.h

+     * is set to 'true' it also allows entries of the form
+     * "key1: value1|value2|value3, etc", in which case the returned
+     * vector will have more entries than the provided
+     * @p list_of_keys.
     *
     * This function also considers a number of special cases:
     * - If the input string consists of only a comma separated


Do you want to add comment here that mentions that you can not do this if allow_subentries == true because then the notation does not provide enough information?

jdannberg · 2019-10-19T00:18:46Z

include/aspect/utilities.h

+     * @param [in] allow_subentries If true allow having multiple subentries
+     *   for each key. If false only allow a single entry per key. In either
+     *   case each key is only allowed to appear once.
+     * @param [out] subentry_structure If handed over this


To me, this name does not seem as descriptive as it could be. What about something like values_per_key or n_values_per_key? That would also be consistent with the naming of the other input parameters.

jdannberg · 2019-10-19T00:26:54Z

unit_tests/parse_map_to_double_array.cc

+    INFO("check 10: ");
+    auto subentry_structure = std::make_shared<std::vector<unsigned int>>();
+
+    compare_vectors_approx(aspect::Utilities::parse_map_to_double_array ("C1:100, C2:200|, C3:300, C4:400, C5:500",


Oh, we actually allow to use the | but then not have an entry? Is that intended?

That is allowed by deal.II (it depends on the implementation of the split_string_list function, which explicitly allows this case). I think this is fine, likely nobody wants to use it.

jdannberg · 2019-10-19T00:29:30Z

unit_tests/parse_map_to_double_array.cc

+  // Subentries not allowed
+  INFO("check fail 7: ");
+  REQUIRE_THROWS_WITH(
+    aspect::Utilities::parse_map_to_double_array ("C1:100, C2:|200, C3:300, C4:400, C5:500",


Wouldn't this test be more helpful and easier to read if the output format was correct (and only the subentries are not allowed)? This way, how do you know which of the two it is?

jdannberg · 2019-10-19T00:35:47Z

source/utilities.cc

+
+          for (const auto &entry: return_map)
+            for (const auto &sub_entry: entry)
+              return_values.push_back(sub_entry);
        }
      else if (Patterns::List(Patterns::Double(),1,n_fields).match(input_string))


We should make sure that allow_subentries is false in this case.

I would not want to do that. Consider the case that somebody wants to allow multiple values per key, but does not know the input structure the user puts into the .prm file. You would still want to allow a simple list of only a single value per key. The parameter is allow_multiple_values_per_key, not enforce_multiple_values_per_key. But you are right, I should document that this format expects exactly one value per key (and we already enforce that, because possibly_extend_from_1_to_N expects either 1 entry, or n_fields entries).

jdannberg · 2019-10-19T00:36:40Z

source/utilities.cc

+                                              "|")
+                               :
+                               Patterns::Double()
+                               ;


Move that to the end of the line before?

jdannberg · 2019-10-19T00:38:22Z

source/utilities.cc

@@ -146,7 +166,12 @@ namespace aspect



There is a space missing in line 162 above.

source/utilities.cc

jdannberg · 2019-10-19T00:55:05Z

include/aspect/utilities.h

+     *   vector will be filled with as many components as
+     *   keys (+1 if there is a background field). Each value represents
+     *   how many subentries were found for this key. The sum over all
+     *   entries is the length of the returned vector of values.
     *
     * @return A vector of values that are parsed from the map, provided
     *   in the order in which the keys appear in the @p list_of_keys argument.


You should add some documentation here that explains the new structure with subentries. That could also be a good place to explain why the subentry_structure is useful (it means whoever calls the function can figure out which value belongs to which key).

gassmoeller · 2019-10-20T18:42:32Z

I think I addressed all comments, and also added the functionality to hand over an existing vector of n_values_per_key that can be used as the expected template for the input parameter. This saves us from implementing Asserts for every parameter to check that its structure is the same as the last one that was parsed. Every change is in the last commit.

jdannberg

Very nice. Please address my two small comments, and then this should be good to go.

jdannberg · 2019-10-21T23:28:04Z

source/utilities.cc

@@ -94,27 +94,37 @@ namespace aspect
                               const std::vector<std::string> &input_field_names,


For some reason, the names of the first two function arguments are different in the declaration and in the implementation here.

jdannberg · 2019-10-21T23:43:51Z

source/utilities.cc

-                (*subentry_structure)[i] = 1;
+                AssertThrow((*n_values_per_key)[i] == 1,
+                            ExcMessage("The input parameter " + property_name + " does not have "
+                                       + "the expected number of values for field index " + std::to_string(i) + "."));


I find this error message a bit confusing. If this error is thrown, it means someone specified a list of doubles in the input file with one value per key, but some other parameter was specified with multiple values per key, provided in the key:value format (or there is some other reason why the function expects more than one value per key).

So to help the user a bit more, I would tell the user how many entries were expected, and tell them something like: "To specify more than one value per field for this input parameter, you need to use the format :value1|value2|..."

gassmoeller · 2019-10-22T20:54:27Z

I addressed the comments. Thanks for the review!

naliboff reviewed Oct 18, 2019

View reviewed changes

source/utilities.cc Outdated Show resolved Hide resolved

Extend function to allow subentries

bc1efcb

gassmoeller force-pushed the expand_parse_map branch from 44390ac to bc1efcb Compare October 19, 2019 00:08

jdannberg reviewed Oct 19, 2019

View reviewed changes

Allow checking subentry_structure

2bcada0

gassmoeller mentioned this pull request Oct 21, 2019

Let phase function handle compositions #3271

Merged

Fix unit tests

b93e969

gassmoeller changed the title ~~Extend parse function to allow subentries~~ Extend parse function to allow multiple values per key Oct 21, 2019

jdannberg approved these changes Oct 21, 2019

View reviewed changes

Address comments

34e9ef4

gassmoeller merged commit 6267cb4 into geodynamics:master Oct 22, 2019

gassmoeller mentioned this pull request Oct 22, 2019

Use phase functions for EoS in visco_plastic material model #3273

Merged

naliboff mentioned this pull request Apr 3, 2020

Add phase changes and compressibility to visco plastic material model #2656

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend parse function to allow multiple values per key #3267

Extend parse function to allow multiple values per key #3267

gassmoeller commented Oct 18, 2019

gassmoeller commented Oct 18, 2019

naliboff left a comment

gassmoeller commented Oct 19, 2019

jdannberg left a comment

jdannberg Oct 19, 2019

jdannberg Oct 19, 2019

jdannberg Oct 19, 2019

gassmoeller Oct 20, 2019

jdannberg Oct 19, 2019

jdannberg Oct 19, 2019

gassmoeller Oct 20, 2019

jdannberg Oct 19, 2019

jdannberg Oct 19, 2019

jdannberg Oct 19, 2019

gassmoeller commented Oct 20, 2019

jdannberg left a comment

jdannberg Oct 21, 2019

jdannberg Oct 21, 2019

gassmoeller commented Oct 22, 2019

		@@ -94,27 +94,37 @@ namespace aspect
		const std::vector<std::string> &input_field_names,

Extend parse function to allow multiple values per key #3267

Extend parse function to allow multiple values per key #3267

Conversation

gassmoeller commented Oct 18, 2019

gassmoeller commented Oct 18, 2019

naliboff left a comment

Choose a reason for hiding this comment

gassmoeller commented Oct 19, 2019

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

gassmoeller commented Oct 20, 2019

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

gassmoeller commented Oct 22, 2019