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

Local coordinate system #2370

Merged
merged 13 commits into from
Mar 9, 2019
Merged

Conversation

endJunction
Copy link
Member

@endJunction endJunction commented Feb 19, 2019

See the project file for the new possibilities. <local_coordinate_system> and a tag in permeability parameter "k" for usage.
The designated parameters will be transformed according to the given local coordinate system.
An additional if-condition is introduced in all parameters, but it has less than 1% impact on the runtime.
Review commit-wise is possible.

TODO:

  • Agree on the input file tags. (decided to keep basis vectors)
  • Documentation for the basis vector (which might change with the above tags todo).
  • Showcase for anisotropic tensor (3D would be preferable, because 2D is already tested in RM)

@endJunction endJunction force-pushed the LocalCoordinateSystem branch 3 times, most recently from ad47ae8 to 2eba549 Compare February 20, 2019 12:06
@codecov
Copy link

codecov bot commented Feb 20, 2019

Codecov Report

Merging #2370 into master will decrease coverage by 0.17%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2370      +/-   ##
==========================================
- Coverage    32.9%   32.73%   -0.18%     
==========================================
  Files         530      530              
  Lines       19816    19850      +34     
  Branches     9258     9325      +67     
==========================================
- Hits         6521     6498      -23     
- Misses       9975     9999      +24     
- Partials     3320     3353      +33
Impacted Files Coverage Δ
Applications/ApplicationsLib/ProjectData.h 0% <ø> (ø) ⬆️
Applications/ApplicationsLib/ProjectData.cpp 0% <0%> (ø) ⬆️
MathLib/Vector3.cpp 33.33% <0%> (-33.34%) ⬇️
MeshLib/CoordinateSystem.cpp 42.85% <0%> (-14.29%) ⬇️
GeoLib/SimplePolygonTree.cpp 61.9% <0%> (-4.77%) ⬇️
GeoLib/AnalyticalGeometry-impl.h 72.95% <0%> (-4.1%) ⬇️
...eStepping/Algorithms/EvolutionaryPIDcontroller.cpp 68.18% <0%> (-3.04%) ⬇️
...ng/Algorithms/IterationNumberBasedTimeStepping.cpp 76.74% <0%> (-2.33%) ⬇️
GeoLib/MinimalBoundingSphere.cpp 41.17% <0%> (-1.97%) ⬇️
MeshLib/MeshSurfaceExtraction.cpp 30.81% <0%> (-1.26%) ⬇️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ec75045...baf3fe9. Read the comment docs.

@JobstM
Copy link

JobstM commented Feb 21, 2019

I just took a brief look on the project file - for me it's ok. One question: How to define multiple local coordinate systems?
Best

@endJunction
Copy link
Member Author

@JobstM Do you mean for different material groups? Or, indeed, multiple, space-dependent coordinate systems?

@JobstM
Copy link

JobstM commented Feb 22, 2019

I mean the possibility to define several coordinate systems, which could be used eg in different areas (material groups) or different material models (eg permeability, elasticity, heat conductivity). Example: A model with 3 material groups, in material group 1, the permeability tensor is inclined by 20 deg, in material group 2 and 3 by 30 deg.

_base[1]->getNumberOfComponents() != 2)
{
OGS_FATAL("The parameters for the 2D basis must have two components.");
}
Copy link
Member

Choose a reason for hiding this comment

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

In case there are wrong input by mistakes, it is also good to check the values of the bases, such as e_i \dot e_j := (i==j) ? 1, 0.

Copy link
Member Author

Choose a reason for hiding this comment

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

Checking the determinant of the transformation matrix to be close to 1. In debug build only, though.

Copy link
Member

Choose a reason for hiding this comment

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

I see.

OGS_FATAL(
"The parameters for the 3D basis must have three components.");
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Here the same, to check the values of the base vectors.

</variables>
</output>
</time_loop>
<local_coordinate_system>
Copy link
Member

Choose a reason for hiding this comment

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

There may be not only one local system in some cases. It might be good to use a nested elements as

<local_coordinate_system>
    <local_coordinate_system>
        <name> x1  </name>
        <basis_vector_0>e0</basis_vector_0>
        <basis_vector_1>e1</basis_vector_1>
    </local_coordinate_system>
</local_coordinate_system>

Copy link
Member Author

Choose a reason for hiding this comment

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

@JobstM Material group dependent coordinate systems are not a problem with a single <local_coordinate_system> tag.This is because the parameters for the base vectors can be group based. An (untested) example:

<local_coordinate_system>
    <basis_vector_0>e0</basis_vector_0>
    <basis_vector_1>e1</basis_vector_1>
</local_coordinate_system>
<parameters>
    <parameter>
        <name>e0</name>
        <type>Group</type>
        <group_id_property>MaterialIDs</group_id_property>
        <index_values>
            <index>0</index>
            <values>1 0 0</value> <!-- e0 for first material -->
        </index_values>
        <index_values>
            <index>1</index>
            <value>0.707 0.707 0</value> <!-- e0 for second material -->
         </index_values>
    </parameter>
    <parameter>
        <name>e1</name>
        <type>Group</type>
        <group_id_property>MaterialIDs</group_id_property>
        <index_values>
            <index>0</index>
            <values>0 1 0</value> <!-- e1 for first material -->
        </index_values>
        <index_values>
            <index>1</index>
            <value>-0.707 0.707 0</value> <!-- e1 for second material -->
         </index_values>
    </parameter>
</parameters>

Copy link
Contributor

Choose a reason for hiding this comment

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

More local_coordinate_system-tags are then only necesary if different parameters for the same material group have distinct principle directions? Or is there a way to represent this in the structure, too?

Copy link
Member

Choose a reason for hiding this comment

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

I see. The current implementation can handle multi-local systems

@ThieJan Like this (figure from the Internet)
untitled

Copy link
Member Author

@endJunction endJunction Feb 22, 2019

Choose a reason for hiding this comment

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

What Jan says is correct, the multiple <local_coordinate_system> tags would only be necessary if, for example, temperature diffusion lives in a different coordinate system than the hydraulic permeability.

Suggest to wait until such a project comes up; the implementation is simple, just a name and instead of boolean <use_local_coordinate_system> a specific coordinate system would be referenced.

Copy link
Member Author

Choose a reason for hiding this comment

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

Another possible input could be:

<local_coordinate_system>
    <basis_matrix>basis</basis_matrix>
</local_coordinate_system>
<parameters>
    <parameter>
        <name>basis</name>
        <type>Group</type>
        <group_id_property>MaterialIDs</group_id_property>
        <index_values>
            <index>0</index>
            <values>1 0 0 0 1 0 0 0 1</value> <!-- basis for first material -->
        </index_values>
        <index_values>
            <index>1</index>
            <values>0.707 0.707 0 -0.707 0.707 0 0 0 1</values> <!--  basis for second material -->
         </index_values>
    </parameter>
</parameters>

@endJunction endJunction force-pushed the LocalCoordinateSystem branch 2 times, most recently from 5d56d4f to 682872c Compare February 22, 2019 14:42
@endJunction endJunction force-pushed the LocalCoordinateSystem branch 3 times, most recently from e0103a1 to ffe4466 Compare March 8, 2019 08:45
@endJunction
Copy link
Member Author

@TomFischer or @wenqing I've added a 3D test for anisotropy but had to extend the GWF. Could you please review the last two commits?


if (it == parameters.end())
{
return nullptr;
Copy link
Member

Choose a reason for hiding this comment

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

Is this a usual case? Maybe add also some debug output to detect if there is an error?

Copy link
Member Author

Choose a reason for hiding this comment

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

The nullptr is checked later by the caller, which issues a warning or whatever.

Copy link
Member

@TomFischer TomFischer left a comment

Choose a reason for hiding this comment

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

Looks good. ⏩

Copy link
Member

@wenqing wenqing left a comment

Choose a reason for hiding this comment

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

The 3D test looks good.

@@ -28,6 +28,34 @@ namespace GroundwaterFlow
{
const unsigned NUM_NODAL_DOF = 1;

template <int GlobalDim>
Eigen::Matrix<double, GlobalDim, GlobalDim> hydraulicConductivity(
Copy link
Member

Choose a reason for hiding this comment

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

Nice. It can be a common function for similar tensor parameters.

@@ -757,3 +757,5 @@ AddTest(
DIFF_DATA
square_1x1_quad_1e5.vtu square_1e5_volumetricsourceterm_pcs_0_ts_1_t_1.000000.vtu analytical_solution pressure 0.75e-4 1e-16
)

Copy link
Member

@wenqing wenqing Mar 8, 2019

Choose a reason for hiding this comment

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

if (NOT OGS_USE_MPI)
    OgsTest(PROJECTFILE "Elliptic/cube_1x1x1_GroundWaterFlow/cube_1e4_anisotropic.prj")
endif()

The truncated version is not exactly comparable with the
coordinate transformation version and results deviate by
10^-7. Better to use accurate representation of the tensor
for the comparisons.
Only for debug mode, because the transformation can not
be efficiently precomputed in the general case.
Calls corresponding to the dimension rotation function
from the coordinate system.
@endJunction
Copy link
Member Author

I've removed the split of findParameterOptional and findParameterByName, because there is a circular dependecy (on header files level) btw. the ProcessLib and MaterialLib. This is handled in a different PR.

@endJunction endJunction merged commit a2f8e2a into ufz:master Mar 9, 2019
@endJunction endJunction deleted the LocalCoordinateSystem branch March 9, 2019 18:24
@ogsbot
Copy link
Member

ogsbot commented Jun 19, 2020

OpenGeoSys development has been moved to GitLab.

See this pull request on GitLab.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants