Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Extend parse function to allow multiple values per key #3267

Merged
merged 4 commits into from Oct 22, 2019
Merged

Extend parse function to allow multiple values per key #3267

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bc1efcb
Extend function to allow subentries
gassmoeller Oct 18, 2019
2bcada0
Allow checking subentry_structure
gassmoeller Oct 19, 2019
b93e969
Fix unit tests
gassmoeller Oct 21, 2019
34e9ef4
Address comments
gassmoeller Oct 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
34 changes: 23 additions & 11 deletions include/aspect/utilities.h
View file
Open in desktop
Expand Up @@ -79,7 +79,7 @@ namespace aspect
* This function takes a string argument that is interpreted as a map
* of the form "key1 : value1, key2 : value2, etc", and then parses
* it to return a vector of these values where the values are ordered
* in the same order as a given set of keys. If @p allow_subentries
* in the same order as a given set of keys. If @p allow_multiple_values_per_key
* 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
Expand All @@ -93,7 +93,8 @@ namespace aspect
* where "key1", "key2", ... are exactly the keys provided by the
* @p list_of_keys in the same order as provided. In this situation,
* if a background field is required, the background value is
* assigned to the first element of the output vector.
* assigned to the first element of the output vector. This form only
* allows a single value per key.
* - Whether or not a background field is required depends on
* the parameter being parsed. Requiring a background field
* increases the size of the output vector by 1. For example,
Expand All @@ -114,25 +115,36 @@ namespace aspect
* @param[in] property_name A name that identifies the type of property
* that is being parsed by this function and that is used in generating
* error messages if the map does not conform to the expected format.
* @param [in] allow_subentries If true allow having multiple subentries
* for each key. If false only allow a single entry per key. In either
* @param [in] allow_multiple_values_per_key If true allow having multiple values
* for each key. If false only allow a single value per key. In either
* case each key is only allowed to appear once.
* @param [out] subentry_structure If handed over this
* 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.
* @param [in,out] n_values_per_key A pointer to a vector of unsigned
* integers. If no pointer is handed over nothing happens. If a pointer
* to an empty vector is handed over, the vector
* is resized with as many components as
* keys (+1 if there is a background field). Each value then stores
* how many values were found for this key. The sum over all
* entries is the length of the return value of this function.
* If a pointer to an existing vector with one or more entries is
* handed over (e.g. a n_values_per_key vector created by a
* previous call to this function) then this vector is used as
* expected structure of the input parameter, and it is checked that
* @p key_value_map fulfills this structure.
*
* @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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

* If multiple values per key are allowed, the vector contains first all
* values for key 1, then all values for key 2 and so forth. Using the
* @p n_values_per_key vector allows the caller to sort entries in the
* returned vector to specific keys.
*/
std::vector<double>
parse_map_to_double_array (const std::string &key_value_map,
const std::vector<std::string> &list_of_keys,
const bool has_background_field,
const std::string &property_name,
const bool allow_subentries = false,
std::shared_ptr<std::vector<unsigned int> > subentry_structure = nullptr);
const bool allow_multiple_values_per_key = false,
std::shared_ptr<std::vector<unsigned int> > n_values_per_key = nullptr);

/**
* This function takes a string argument that is assumed to represent
Expand Down
74 changes: 55 additions & 19 deletions source/utilities.cc
View file
Open in desktop
Expand Up @@ -94,27 +94,37 @@ namespace aspect
const std::vector<std::string> &input_field_names,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const bool has_background_field,
const std::string &property_name,
const bool allow_subentries,
std::shared_ptr<std::vector<unsigned int> > subentry_structure)
const bool allow_multiple_values_per_key,
std::shared_ptr<std::vector<unsigned int> > n_values_per_key)
{
std::vector<std::string> field_names = input_field_names;
if (has_background_field)
field_names.insert(field_names.begin(),"background");

const unsigned int n_fields = field_names.size();
std::vector<double> return_values;
if (subentry_structure)
subentry_structure->resize(n_fields,0);

const auto key_pattern = (allow_subentries)
const bool check_structure = (n_values_per_key && n_values_per_key->size() != 0);
const bool store_structure = (n_values_per_key && n_values_per_key->size() == 0);

if (store_structure)
n_values_per_key->resize(n_fields,0);

if (check_structure)
AssertThrow(n_values_per_key->size() == n_fields,
ExcMessage("When providing an expected structure for input parameter " + property_name + " you need to provide "
+ "as many entries in the structure vector as there are input field names (+1 if there is a background field). "
+ "The current structure vector has " + std::to_string(n_values_per_key->size()) + " entries, but there are "
+ std::to_string(n_fields) + " field names." ));

const auto key_pattern = (allow_multiple_values_per_key)
?
Patterns::List(Patterns::Double(),
1,
std::numeric_limits<unsigned int>::max(),
"|")
:
Patterns::Double()
;
Patterns::Double();

// Parse the string depending on what Pattern we are dealing with
if (Patterns::Map(Patterns::Anything(),
Expand Down Expand Up @@ -159,18 +169,29 @@ namespace aspect
ExcMessage ("There is only one "
+ property_name
+ " value given. The keyword `all' is "
"expected but is not found. Please"
"expected but is not found. Please "
"check your "
+ property_name
+ " list."));

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a space missing in line 162 above.

const std::vector<std::string> values = dealii::Utilities::split_string_list(key_and_value[1], '|');

// Assign all the elements to the "all" value
for (unsigned int field_index=0; field_index<n_fields; ++field_index)
{
return_map[field_index].push_back(Utilities::string_to_double(key_and_value[1]));
for (const auto &value : values)
{
return_map[field_index].push_back(Utilities::string_to_double(value));

if (store_structure)
++(*n_values_per_key)[field_index];
}

if (subentry_structure)
(*subentry_structure)[field_index] = 1;
if (check_structure)
AssertThrow((*n_values_per_key)[field_index] == values.size(),
ExcMessage("The key <" + key_and_value[0] + "> in <"+ property_name + "> does not have "
+ "the expected number of values. It expects " + std::to_string((*n_values_per_key)[field_index])
+ " values, but we found " + std::to_string(values.size()) + " values."));
}
gassmoeller marked this conversation as resolved.
Show resolved Hide resolved
}
// Handle lists of multiple entries
Expand Down Expand Up @@ -237,15 +258,21 @@ namespace aspect
"is not set, the default names are "
"C_1, C_2, ..., C_n."));

const std::vector<std::string> subfield_entries = dealii::Utilities::split_string_list(key_and_value[1], '|');
const std::vector<std::string> values = dealii::Utilities::split_string_list(key_and_value[1], '|');

for (const auto &entry : subfield_entries)
for (const auto &value : values)
{
return_map[field_index].push_back(Utilities::string_to_double(entry));
return_map[field_index].push_back(Utilities::string_to_double(value));

if (subentry_structure)
++(*subentry_structure)[field_index];
if (store_structure)
++(*n_values_per_key)[field_index];
}

if (check_structure)
AssertThrow((*n_values_per_key)[field_index] == values.size(),
ExcMessage("The key <" + key_and_value[0] + "> in <"+ property_name + "> does not have "
+ "the expected number of values. It expects " + std::to_string((*n_values_per_key)[field_index])
+ " values, but we found " + std::to_string(values.size()) + " values."));
}
}

Expand All @@ -260,10 +287,18 @@ namespace aspect
n_fields,
property_name);

if (subentry_structure)
if (store_structure)
{
for (unsigned int i=0; i<n_fields; ++i)
(*n_values_per_key)[i] = 1;
}

if (check_structure)
{
for (unsigned int i=0; i<n_fields; ++i)
(*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) + "."));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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|..."

}
}
else
Expand All @@ -273,7 +308,8 @@ namespace aspect
ExcMessage ("The required format for field <"
+ property_name
+ "> was not found. Specify a comma separated "
"list of `<double>' or `<id> : <double>|<double>|...'."));
+ "list of `<double>' or `<key1> : <double>|<double>|..., "
+ "<key2> : <double>|... , ... '."));
}
return return_values;
}
Expand Down
111 changes: 102 additions & 9 deletions unit_tests/parse_map_to_double_array.cc
View file
Open in desktop
Expand Up @@ -84,32 +84,78 @@ TEST_CASE("Utilities::parse_map_to_double_array")

{
INFO("check 9: ");
auto subentry_structure = std::make_shared<std::vector<unsigned int>>();
auto n_values_per_key = std::make_shared<std::vector<unsigned int>>();

compare_vectors_approx(aspect::Utilities::parse_map_to_double_array ("C1:100, C2:200|100, C3:300, C4:400, C5:500, background:0",
{"C1","C2","C3","C4","C5"},
true,
"TestField",
true,
subentry_structure),
n_values_per_key),
{0.0,100.0,200.0,100.0,300.0,400.0,500.0});

REQUIRE(*subentry_structure == std::vector<unsigned int>({1,1,2,1,1,1}));
REQUIRE(*n_values_per_key == std::vector<unsigned int>({1,1,2,1,1,1}));
}

{
INFO("check 10: ");
auto subentry_structure = std::make_shared<std::vector<unsigned int>>();
auto n_values_per_key = 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",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{"C1","C2","C3","C4","C5"},
false,
"TestField",
true,
subentry_structure),
n_values_per_key),
{100.0,200.0,300.0,400.0,500.0});

REQUIRE(*subentry_structure == std::vector<unsigned int>({1,1,1,1,1}));
REQUIRE(*n_values_per_key == std::vector<unsigned int>({1,1,1,1,1}));
}

{
INFO("check 11: ");
auto n_values_per_key = std::make_shared<std::vector<unsigned int>>();

compare_vectors_approx(aspect::Utilities::parse_map_to_double_array ("C1:100, C2:200|300, C3:300, C4:400, C5:500",
{"C1","C2","C3","C4","C5"},
false,
"TestField",
true,
n_values_per_key),
{100.0,200.0,300.0,300.0,400.0,500.0});

REQUIRE(*n_values_per_key == std::vector<unsigned int>({1,2,1,1,1}));

compare_vectors_approx(aspect::Utilities::parse_map_to_double_array ("C1:100, C2:200|300, C3:300, C4:400, C5:500",
{"C1","C2","C3","C4","C5"},
false,
"TestField",
true,
n_values_per_key),
{100.0,200.0,300.0,300.0,400.0,500.0});
}

{
INFO("check 12: ");
auto n_values_per_key = std::make_shared<std::vector<unsigned int>>();

compare_vectors_approx(aspect::Utilities::parse_map_to_double_array ("all:300|400",
{"C1","C2"},
false,
"TestField",
true,
n_values_per_key),
{300.0,400.0,300.0,400.0});

REQUIRE(*n_values_per_key == std::vector<unsigned int> ({2,2}));

compare_vectors_approx(aspect::Utilities::parse_map_to_double_array ("all:100|200",
{"C1","C2"},
false,
"TestField",
true,
n_values_per_key),
{100.0,200.0,100.0,200.0});
}

INFO("check complete");
Expand Down Expand Up @@ -169,13 +215,13 @@ TEST_CASE("Utilities::parse_map_to_double_array FAIL ON PURPOSE")
// 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",
aspect::Utilities::parse_map_to_double_array ("C1:100, C2:100|200, C3:300, C4:400, C5:500",
{"C1","C2","C3","C4","C5"},
false,
"TestField"), Contains("The required format for field"));

// Wrong subentry format
INFO("check fail 7: ");
INFO("check fail 8: ");
REQUIRE_THROWS_WITH(
aspect::Utilities::parse_map_to_double_array ("C1:100, C2:|200, C3:300, C4:400, C5:500",
{"C1","C2","C3","C4","C5"},
Expand All @@ -184,13 +230,60 @@ TEST_CASE("Utilities::parse_map_to_double_array FAIL ON PURPOSE")
true), Contains("The required format for field"));

// No subentries
INFO("check fail 8: ");
INFO("check fail 9: ");
REQUIRE_THROWS_WITH(
aspect::Utilities::parse_map_to_double_array ("C1:100, C2:|, C3:300, C4:400, C5:500",
{"C1","C2","C3","C4","C5"},
false,
"TestField",
true), Contains("The required format for field"));

// Wrong input structure
{
INFO("check fail 10: ");
auto n_values_per_key = std::make_shared<std::vector<unsigned int>>();

compare_vectors_approx(aspect::Utilities::parse_map_to_double_array ("C1:100, C2:200|300, C3:300, C4:400, C5:500",
{"C1","C2","C3","C4","C5"},
false,
"TestField",
true,
n_values_per_key),
{100.0,200.0,300.0,300.0,400.0,500.0});

REQUIRE(*n_values_per_key == std::vector<unsigned int>({1,2,1,1,1}));

REQUIRE_THROWS_WITH(aspect::Utilities::parse_map_to_double_array ("C1:100|200, C2:300, C3:300, C4:400, C5:500",
{"C1","C2","C3","C4","C5"},
false,
"TestField",
true,
n_values_per_key),
Contains("the expected number of entries"));
}

{
INFO("check fail 11: ");
auto n_values_per_key = std::make_shared<std::vector<unsigned int>>();

compare_vectors_approx(aspect::Utilities::parse_map_to_double_array ("all:300|400",
{"C1","C2"},
false,
"TestField",
true,
n_values_per_key),
{300.0,400.0,300.0,400.0});

REQUIRE(*n_values_per_key == std::vector<unsigned int>({2,2}));

REQUIRE_THROWS_WITH(aspect::Utilities::parse_map_to_double_array ("all:100|200|300",
{"C1","C2"},
false,
"TestField",
true,
n_values_per_key),
Contains("the expected number of entries"));
}
}


Expand Down