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.

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

[MatL] Multi-phase/-component library. #2303

Merged
merged 4 commits into from Jan 17, 2019

Conversation

Projects
None yet
4 participants
@endJunction
Copy link
Member

endJunction commented Dec 21, 2018

This is part of #1955 which is used in multi component transport (#2304). Mainly infrastructure, since only constant properties were used so far in the ComponentTransportProcess.

@endJunction endJunction force-pushed the endJunction:gpml branch from 9aa48a5 to 3064216 Dec 21, 2018

@endJunction endJunction force-pushed the endJunction:gpml branch 3 times, most recently from 9a47f59 to b1c7d3b Jan 2, 2019

@TomFischer

This comment has been minimized.

Copy link
Member

TomFischer commented Jan 8, 2019

The copyright has to be updated.

/// determine the size of the VariableArray. If the variable of your choice is
/// missing, simply add it somewhere at the list, but above the last entry
/// 'numberOfVariables'
enum Variables : std::size_t

This comment has been minimized.

@TomFischer

TomFischer Jan 8, 2019

Member

Why not 'enum class'?

This comment has been minimized.

@endJunction

endJunction Jan 10, 2019

Author Member

The implicit conversion to int (I changed the underlying type) is quite handy.

#pragma once

#include <boost/algorithm/string/predicate.hpp>
#include <boost/variant.hpp>

This comment has been minimized.

@TomFischer

TomFischer Jan 8, 2019

Member

Can we already use std::variant?

This comment has been minimized.

@endJunction

endJunction Jan 10, 2019

Author Member

Unfortunately not until gcc-7.1. And also the macOS's clang in version 10 does not support it fully yet. But (fasten your belts!!!) the latest visual studio would work!

Also tried new syntax for "structured binding declarations" instead of std::tie

auto [a, b, c] = function_returning_a_tuple_of_tree();

working only since gcc-7

/// in alphabetical order (of course, except for the last entry
/// 'number_of_property_enums'). Please note that any of these entries must also
/// appear in below convert functions.
enum PropertyType : std::size_t

This comment has been minimized.

@TomFischer

TomFischer Jan 8, 2019

Member

Why not enum class?

Show resolved Hide resolved MaterialLib/MPL/Enums.h Outdated
class MaterialSpatialDistributionMap
{
private:
std::map<int, std::unique_ptr<Medium>> const& _media;

This comment has been minimized.

@TomFischer

TomFischer Jan 8, 2019

Member

As far as I understand the domain of the map is [0, #number of media]. Maybe a vector can be used here?

This comment has been minimized.

@endJunction

endJunction Jan 8, 2019

Author Member

Note to myself; compare with error/warning message code.

This comment has been minimized.

@endJunction

endJunction Jan 10, 2019

Author Member

@TomFischer the media ids follow numbering from MaterialIDs and can be rather arbitrary, especially negative. We might have a look on it later, if it looks like a bottleneck.

Show resolved Hide resolved MaterialLib/MPL/Medium.h Outdated
@@ -26,6 +26,7 @@
#include "BaseLib/FileTools.h"

#include "GeoLib/GEOObjects.h"
#include "MaterialLib/MPL/mpMedium.h"

This comment has been minimized.

@wenqing

wenqing Jan 8, 2019

Member

Not for this PR: In future I would like to replace MPL with MaterialPropertyLib. Also, it is better to replace mpMedium.h` with some meaningful name.

Show resolved Hide resolved Applications/ApplicationsLib/ProjectData.cpp Outdated
if (_media.rbegin()->first != static_cast<int>(_media.size()) - 1)
OGS_FATAL(
"The ids in the porous media definitions in the project file have "
"to be sequential, starting with the id zero.");

This comment has been minimized.

@wenqing

wenqing Jan 8, 2019

Member

the id zero --> the id of zero.

This comment has been minimized.

@endJunction

endJunction Jan 10, 2019

Author Member

I dropped the whole if-condition. The requirement of media to be numbered from 0 to n sequentially, does not match with the freedom of the MaterialIDs, which can be any int, especially negative.

{
// If a name is given, it must conform with one of the derived component
// names in the following list:
if (boost::iequals(component_name, "water"))

This comment has been minimized.

@wenqing

wenqing Jan 8, 2019

Member

Why not if (component_name == "water")?

This comment has been minimized.

@endJunction

endJunction Jan 10, 2019

Author Member

Norbert's argument is that it is difficult to explain people, why "Water" is not good enough for the specification of water. Same goes with "CO2" etc.

*/
#pragma once

#include "BaseLib/ConfigTree.h"

This comment has been minimized.

@wenqing

wenqing Jan 8, 2019

Member

can use forward declaration.

This comment has been minimized.

@endJunction

endJunction Jan 10, 2019

Author Member

Whole creation of media, phases, components, and properties is now moved into corresponding CreateX.h/cpp files.

@@ -0,0 +1,25 @@
/**
* \file

This comment has been minimized.

@wenqing

wenqing Jan 8, 2019

Member

remove it or leave a blank line after it.

*/
#pragma once

#include "Constant.h"

This comment has been minimized.

@wenqing

wenqing Jan 8, 2019

Member

This file only contains one include.

This comment has been minimized.

@endJunction

endJunction Jan 10, 2019

Author Member

Same as for components, the full implementation has many, and the properties include is specified once in the process.
Other properties (and also components) which are not tested in the follow-up PR, are excluded.

#include <boost/variant.hpp>
#include <string>

#include "BaseLib/ConfigTree.h"

This comment has been minimized.

@wenqing

wenqing Jan 8, 2019

Member

Forward declaration.

@@ -0,0 +1,21 @@
/**
* \file

This comment has been minimized.

@wenqing

wenqing Jan 8, 2019

Member

Leave a blank line after it.

Show resolved Hide resolved Tests/MaterialLib/TestMPLParseMaterial.cpp Outdated
Show resolved Hide resolved MaterialLib/MPL/Component.cpp Outdated
};

using PropertyDataType =
boost::variant<double, Pair, Vector, SymmTensor, Tensor, std::string>;

This comment has been minimized.

@TomFischer

TomFischer Jan 9, 2019

Member

I think std::variant can be used.

@endJunction endJunction force-pushed the endJunction:gpml branch 5 times, most recently from b878684 to 90364f4 Jan 9, 2019

@wenqing
Copy link
Member

wenqing left a comment

Looks good! Only one idea about MaterialPropertyLib::PropertyType and its converters.

std::stringstream issValue(sValue);

std::vector<double> values;
double dummy(0);

This comment has been minimized.

@wenqing

wenqing Jan 14, 2019

Member

const

This comment has been minimized.

@TomFischer

TomFischer Jan 16, 2019

Member

dummy cann't be const since it is used to read from the stringstream issValues. So maybe 'dummy' isn't so dummy ;-)

This comment has been minimized.

@endJunction

endJunction Jan 16, 2019

Author Member

Didn't see the reading of std::vector<double> from the config. ConfigTree supports getConfigParameter<std::vector<T>>(...). The whole section is replaced by single statement.

/// I suggest that the syntax of the properties in the input-files (i.e. the
/// strings) have to be identical to the syntax of the entries in the
/// enumerator.
inline PropertyType convertStringToProperty(std::string const& inString)

This comment has been minimized.

@wenqing

wenqing Jan 14, 2019

Member

If we want to the elements of PropertyType, we have to to change strings in convertStringToProperty and property_enum_to_string as well.

It might be good to
1) create a enum to std::string convertor in BaseLib (see https://stackoverflow.com/questions/28828957/enum-to-string-in-modern-c11-c14-c17-and-future-c20).
2) use the enum to std::string convertor to initialize std::array<std::string, PropertyType::number_of_properties> property_enum_to_string
3) create a std::map<std::string, PropertyType> by using of std::string convertor to replace ~convertStringToProperty ~

With that, we only need to change the elements of PropertyType if such change is needed.

The current implementation is also OK because there might be less change in PropertyType.

This comment has been minimized.

@wenqing

wenqing Jan 15, 2019

Member

1) is not good. One still has to use the enum name to get its string type. It seems that each name of enum element must appears at least two times in the source code. Therefore, please go forward with the present implementation.

std::unique_ptr<PropertyArray>&& properties);
virtual ~Component() = default;

/// A get-function for retrieving a cartain property.

This comment has been minimized.

@TomFischer

TomFischer Jan 15, 2019

Member

cartain -> certain

return property(p).template value<T>();
}

template <typename T>

This comment has been minimized.

@TomFischer

TomFischer Jan 15, 2019

Member

Is my understanding correct: the variable_array is used to pass information for property value calculation?

This comment has been minimized.

@endJunction

endJunction Jan 16, 2019

Author Member

Yes.

ERR("Phase type should be one of:");
for (auto const type : allowed_phase_types)
{
ERR(type.c_str());

This comment has been minimized.

@TomFischer

TomFischer Jan 15, 2019

Member

Is this valid? I thought it must be ERR("%s", type.c_str()).

This comment has been minimized.

@endJunction

endJunction Jan 16, 2019

Author Member

It's just a char array, with substitution or without.

return p.value().which();
}

inline void overwriteExistingProperties(PropertyArray& properties,

This comment has been minimized.

@TomFischer

TomFischer Jan 16, 2019

Member

Since in line 111 new_properties entries are moved, the new_properties aren't valid anymore, isn't it? Maybe the name of the function is a bit misleading, but I haven't a good suggestion.

This comment has been minimized.

@endJunction

endJunction Jan 16, 2019

Author Member

Good point. Need a name.


/// Simple symmetric tensor data type for holding
/// xx, yy, zz, xy, xz, yz tensor components.
using SymmTensor = std::array<double, 9>;

This comment has been minimized.

@TomFischer

TomFischer Jan 16, 2019

Member

The documentation says there are 6 entries, the array has size 9. Is this correct?

This comment has been minimized.

@endJunction

endJunction Jan 16, 2019

Author Member

Good catch! fixing.

}

/// This method creates an XML-snippet of a sigle property.
std::string makeProperty(std::size_t ind, std::size_t index,

This comment has been minimized.

@TomFischer

TomFischer Jan 16, 2019

Member

Why not ... makeProperty(std::size_t const ind, std::size_t const index, std::string const& property)?

{
std::stringstream sProperty;

if (property == "")

This comment has been minimized.

@TomFischer

TomFischer Jan 16, 2019

Member

Maybe property.empty() would be more readable.

// What follows is a C-style string-to-double-conversion...
// seems so easy compared to c++-standard; Feel free to
// change it to latest standard..
if (double value = atof(property.c_str()))

This comment has been minimized.

@TomFischer

TomFischer Jan 16, 2019

Member

std::atof please.

return sComponents.str();
}

/// This method creates an XML-snippet of a sigle material

This comment has been minimized.

@TomFischer

TomFischer Jan 16, 2019

Member

sigle -> single


/// This method creates an XML-snippet of the material
/// phases.
std::string makePhases(std::vector<Phase> phases)

This comment has been minimized.

@TomFischer

TomFischer Jan 16, 2019

Member

std::vector<Phase> const& phases?

/// This method creates the entire XML-tree structure from the string-
/// based medium specification object. I know, indentation is not
/// necessary for this, but my OCD kicked in...
std::string makeMedium(Medium m)

This comment has been minimized.

@TomFischer

TomFischer Jan 16, 2019

Member

Medium const& m

std::vector<std::string>* exp)
{
obs->push_back(observed_name);
if (expectation == "")

This comment has been minimized.

@TomFischer

TomFischer Jan 16, 2019

Member

IMHO expectation.empty() would be more readable.

@TomFischer
Copy link
Member

TomFischer left a comment

Looks good.

private:
std::vector<std::unique_ptr<Component>> const _components;

/// Here, all for all properties listed in the Properties enumerator are

This comment has been minimized.

@TomFischer

TomFischer Jan 16, 2019

Member

all for

@endJunction

This comment has been minimized.

Copy link
Member Author

endJunction commented Jan 16, 2019

@TomFischer I polished the TestMPLParseMaterial.cpp with all the const, &, typos, etc. being fixed now.

@endJunction

This comment has been minimized.

Copy link
Member Author

endJunction commented Jan 16, 2019

Two open points remain: enum <-> string, and name for overwriteProperties.

NoCodeYet and others added some commits Sep 7, 2017

[MatL] Major rewrite.
A lot of changes are renames for consistency with OGS.

Splitting files, and rename them (dropping mp, p, c prefixes).

Extraction of the create* functions lead to single step initialization
of the Medium, Phase, Component, and Property objects.

Formatting and clang-tidy.
Improve const-correctness.

Remove unneeded Undefined property: The nullptr is probably sufficient
in the PropertyArray, which is an array of pointers, for the property to
be undefined.

Unify value/dValue/d2Value access.

[App] Remove requirement for media ids sequence.

Reorder public, protected, private sections.

Add missing ogs_file_param documentation lines in the code.

@endJunction endJunction force-pushed the endJunction:gpml branch from 6059565 to c5442ac Jan 17, 2019

@endJunction

This comment has been minimized.

Copy link
Member Author

endJunction commented Jan 17, 2019

When there are no further comments, and the builds pass, this can be merged.
The enum <-> string conversion needs additional discussions first.

@wenqing

This comment has been minimized.

Copy link
Member

wenqing commented Jan 17, 2019

@TomFischer TomFischer merged commit 8972d34 into ufz:master Jan 17, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
deploy/netlify Deploy preview ready!
Details

@endJunction endJunction deleted the endJunction:gpml branch Jan 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.