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

[L-IE] supporting fracture intersections in mechanics #2235

Merged
merged 21 commits into from Dec 10, 2018

Conversation

Projects
None yet
3 participants
@norihiro-w
Copy link
Collaborator

norihiro-w commented Oct 8, 2018

supporting branch/junction intersections of fractures in 2D M problems

@bilke bilke force-pushed the ufz:master branch from 7e4d94d to 26445e2 Nov 16, 2018

@norihiro-w norihiro-w force-pushed the norihiro-w:lie-intersects-pr branch 3 times, most recently from 8b582ba to bb1a621 Nov 30, 2018

@norihiro-w

This comment has been minimized.

Copy link
Collaborator Author

norihiro-w commented Dec 3, 2018

i can't find out why Jenkins is failing. anyone can help me?

@endJunction

This comment has been minimized.

Copy link
Member

endJunction commented Dec 3, 2018

I think it's a temporary internet access error:
Get https://registry-1.docker.io/v2/library/ubuntu/manifests/16.04: received unexpected HTTP status: 503 Service Unavailable
Just restarted appveyor and jenkins.

@norihiro-w

This comment has been minimized.

Copy link
Collaborator Author

norihiro-w commented Dec 3, 2018

@endJunction can you please restart the Jenkins again?

@endJunction

This comment has been minimized.

Copy link
Member

endJunction commented Dec 3, 2018

The jenkins run also fine, but has new warnings; Github cannot display the "unstable" build, so it is displaying a failure. The warnings are:
on windows: for postutils.cpp: 59 and 67: 'return': conversion from '__int64' to 'int', possible loss of data and 'return': conversion from 'const __int64' to 'unsigned int', possible loss of data
Then there are new warning for doxygen in levelsetfunction.h; there the @parm is wrong, should be @param.

The builds automatically starts, if there is a change in commit hash, e.g. by changing the last commit's date.

@norihiro-w

This comment has been minimized.

Copy link
Collaborator Author

norihiro-w commented Dec 4, 2018

@endJunction thanks!

@norihiro-w

This comment has been minimized.

Copy link
Collaborator Author

norihiro-w commented Dec 4, 2018

i think now this PR is ready for reviewing

// unit vector normal to the master fracture in a direction to the slave
Eigen::Vector3d normal_vector_branch;

virtual ~BranchProperty() = default;

This comment has been minimized.

@endJunction

endJunction Dec 4, 2018

Member

This comment has been minimized.

@norihiro-w

norihiro-w Dec 6, 2018

Author Collaborator

corrected

MeshLib::Node const& branchNode,
FractureProperty const& master_frac,
FractureProperty const& slave_frac,
BranchProperty& branch)

This comment has been minimized.

@endJunction

endJunction Dec 4, 2018

Member

Consideration: I'd rather return a new Branch object, or maybe create a constructor to the Branch class.

This comment has been minimized.

@norihiro-w

norihiro-w Dec 7, 2018

Author Collaborator

done


namespace
{
// Heaviside step function
inline double Heaviside(double v)
{
return (v < 0.0) ? 0.0 : 1.0;
return (v < 0.0) ? 0.0 : 1.0;

This comment has been minimized.

@endJunction

endJunction Dec 4, 2018

Member

It seems, that the clang-format is not using the .clang-format from the source root...

This comment has been minimized.

@norihiro-w

norihiro-w Dec 6, 2018

Author Collaborator

will check it

}

template <class T_VEC, class T_V>
inline bool inList(T_VEC const& vec, T_V const& v)

This comment has been minimized.

@endJunction

endJunction Dec 4, 2018

Member

Would suggest to move it to BaseLib/Algorithm.h, and rename to contains or similar, because it is not acting on lists only.

This comment has been minimized.

@norihiro-w

norihiro-w Dec 6, 2018

Author Collaborator

corrected

std::unordered_map<int,int> const& fracID_to_local,
Eigen::Vector3d const& x)
{
auto this_frac_local_index = fracID_to_local.at(this_frac_id);

This comment has been minimized.

@endJunction

endJunction Dec 4, 2018

Member

Not sure if bound checked access is intended; if not use operator[].

This comment has been minimized.

@norihiro-w

norihiro-w Dec 7, 2018

Author Collaborator

done.

This comment has been minimized.

@norihiro-w

norihiro-w Dec 7, 2018

Author Collaborator

update: i remember it now. i had to use at() because fracID_to_local is const. operator[] cannot be used because it is non-const.

frac_nodeID_to_matIDs[node->getID()].push_back(vec_fracture_mat_IDs[i]);

auto opt_material_ids(
mesh.getProperties().getPropertyVector<int>("MaterialIDs"));

This comment has been minimized.

@endJunction

endJunction Dec 4, 2018

Member

There is MeshLib::materialIDs(mesh) function returning a pointer with additional checks of the property.

This comment has been minimized.

@norihiro-w

norihiro-w Dec 6, 2018

Author Collaborator

done

return (itr2 != vec_nodes.end());
}

std::vector<int> const& getmatids(

This comment has been minimized.

@endJunction

endJunction Dec 4, 2018

Member

longer names are preferred and use camelCase for function names, e.g. getMaterialIdsForNode(...)

This comment has been minimized.

@norihiro-w

norihiro-w Dec 6, 2018

Author Collaborator

done


master_frac.branches_master.emplace_back(branch);

BranchProperty* branch2 = new BranchProperty();

This comment has been minimized.

@endJunction

endJunction Dec 4, 2018

Member

In the new code new is not welcome (unless you have to as in copying the MeshLib's nodes). I also didn't see the reason to store pointers in the *.branches_master/slave vectors instead of the branch objects.
In any case this is a memory leak.

I was wrong, no memory leak, because the branches_master/slave are vectors of unique_ptr's.
Still the question remains, why store pointers instead of values?

This comment has been minimized.

@norihiro-w

norihiro-w Dec 7, 2018

Author Collaborator

Because the number of branches varies depending on problem, i store them as a vector of its pointers. i am not sure if I understand your question.

{
struct BranchProperty
{
int node_id;

This comment has been minimized.

@wenqing

wenqing Dec 5, 2018

Member

These members can be set as const by introducing a constructor.

This comment has been minimized.

@norihiro-w

norihiro-w Dec 7, 2018

Author Collaborator

yes it can be const but i don't see a definitive reason for it.

computeNormalVector(e, dim, frac_prop.normal_vector);
frac_prop.R.resize(dim, dim);
computeRotationMatrix(e, frac_prop.normal_vector, dim, frac_prop.R);
}


inline void setBranchProperty(

This comment has been minimized.

@wenqing

wenqing Dec 5, 2018

Member

can be converted to a constructor of BranchProperty.

This comment has been minimized.

@norihiro-w

norihiro-w Dec 7, 2018

Author Collaborator

here i intentionally separate data and functions (to let myself focus on the data structure during coding).

This comment has been minimized.

@wenqing

wenqing Dec 7, 2018

Member

I see.

@norihiro-w norihiro-w force-pushed the norihiro-w:lie-intersects-pr branch 2 times, most recently from 5e27400 to f8cf098 Dec 6, 2018

@wenqing
Copy link
Member

wenqing left a comment

continue my previous review

computeNormalVector(e, dim, frac_prop.normal_vector);
frac_prop.R.resize(dim, dim);
computeRotationMatrix(e, frac_prop.normal_vector, dim, frac_prop.R);
}

inline BranchProperty* createBranchProperty(MeshLib::Node const& branchNode,

This comment has been minimized.

@wenqing

wenqing Dec 7, 2018

Member

I see that this function is called to fill _process_data._vec_junction_property which is a vector of unique_ptr. I think that you can return std::unique_ptr<BranchProperty> in this function. Then use std::move in line 139 of file
ProcessLib/LIE/SmallDeformation/SmallDeformationProcess.cpp.

This comment has been minimized.

@norihiro-w

norihiro-w Dec 8, 2018

Author Collaborator

i don’t like to use unique ptr too much. current implementation is already readable.

This comment has been minimized.

@wenqing

wenqing Dec 10, 2018

Member

It is just related to the operator of new. If a new is used, one may try to find a delete. If you prefer to use it, it is OK. All other stuffs are OK for me too.

This comment has been minimized.

@wenqing

wenqing Dec 10, 2018

Member

benchmarks failed.

FractureProperty const& master_frac,
FractureProperty const& slave_frac)
{
BranchProperty* branch = new BranchProperty();

This comment has been minimized.

@wenqing

wenqing Dec 7, 2018

Member

new can be replace with make_unique if the return type is change to unique_ptr.

This comment has been minimized.

@norihiro-w

norihiro-w Dec 8, 2018

Author Collaborator

same as above

return branch;
}

inline JunctionProperty* createJunctionProperty(

This comment has been minimized.

@wenqing

wenqing Dec 7, 2018

Member

Here the same.

for (std::size_t i = 0; i < junction_props.size(); i++)
{
auto const* junction = junction_props[i];
auto fid1 = fracID_to_local.at(junction->fracture_IDs[0]);

This comment has been minimized.

@wenqing

wenqing Dec 7, 2018

Member

It is good to add const to these three definitions.

This comment has been minimized.

@norihiro-w

norihiro-w Dec 8, 2018

Author Collaborator

i don’t see reason for it, though fid1 is already const

fracID_to_local.end())
continue;

double singned = boost::math::sign(

This comment has been minimized.

@wenqing

wenqing Dec 7, 2018

Member

const

This comment has been minimized.

@norihiro-w

norihiro-w Dec 8, 2018

Author Collaborator

not important as the scope is limited

if (!BaseLib::contains(junction->fracture_IDs, this_frac.fracture_id))
continue;

auto another_frac_id =

This comment has been minimized.

@wenqing

wenqing Dec 7, 2018

Member

const

This comment has been minimized.

@norihiro-w

norihiro-w Dec 8, 2018

Author Collaborator

not important as the scope is limited

@@ -72,6 +76,11 @@ class HydroMechanicsLocalAssemblerMatrixNearFracture

static const int displacement_jump_index =
displacement_index + displacement_size;

std::vector<FractureProperty*> _fracture_props;

This comment has been minimized.

@wenqing

wenqing Dec 7, 2018

Member

Might be good to use std::vector<std::reference_wrapper<const unique_ptr> > because the vector is pushed with process_data.fracture_property.get().

This comment has been minimized.

@norihiro-w

norihiro-w Dec 8, 2018

Author Collaborator

thanks for the suggestion. however that way is too complicated for me

@@ -75,9 +79,9 @@ void HydroMechanicsLocalAssemblerMatrixNearFracture<

// levelset value of the element
// remark: this assumes the levelset function is uniform within an element
auto const& fracture_props = *_process_data.fracture_property;

This comment has been minimized.

@wenqing

wenqing Dec 7, 2018

Member

fracture_props was set as member of the class as _fracture_props. However I could not found where _fracture_props is initialized. May be I am wrong.

This comment has been minimized.

@norihiro-w

norihiro-w Dec 9, 2018

Author Collaborator

fracture_property is initialized in CreateSmallDeformationProcess.cpp and moved to the process data. see https://github.com/ufz/ogs/blob/master/ProcessLib/LIE/SmallDeformation/CreateSmallDeformationProcess.cpp#L143

This comment has been minimized.

@wenqing

wenqing Dec 10, 2018

Member

I see that is initialized after creating its owner.

@norihiro-w norihiro-w closed this Dec 8, 2018

@norihiro-w norihiro-w reopened this Dec 8, 2018

@endJunction endJunction force-pushed the norihiro-w:lie-intersects-pr branch from 7e27f44 to e3d6fa9 Dec 10, 2018

@endJunction endJunction force-pushed the norihiro-w:lie-intersects-pr branch from 4904a1b to 46cc9d2 Dec 10, 2018

@endJunction endJunction merged commit bfa0fdf into ufz:master Dec 10, 2018

2 of 3 checks passed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
deploy/netlify Deploy preview ready!
Details
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.