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

Python BCs #2170

Merged
merged 23 commits into from Aug 17, 2018

Conversation

Projects
None yet
5 participants
@chleh
Copy link
Collaborator

chleh commented Jul 31, 2018

Python code for BCs

Limitations (not solved in this PR)

  • Only works if all physical fields have the same ansatz functions, i.e., not for HM.
  • The normal traction BC is special and not covered by this PR.
  • Constraint boundary conditions (#2145) cope with secondary variables (fluxes). Fluxes cannot be passed to the Python scripts by the code of this PR.

TODO

  • finish Hertz contact test
  • test with nonlinear BC
  • test with multi-variable process
  • input file documentation
  • maybe fix possible Python version mismatch (presumably between VTK and OGS)
  • fix git lfs tricks (*.pnK)
  • squash

Review

  • attribute used
  • VTK Python version mismatch
  • discuss NLSolver changes.
  • Large tests & Python?

Issues to open

  • Maybe the two versions of deriveBCMap could be merged into one?
  • Python BC for normal traction
  • Python BC for mixed FEM ansatz functions
  • passing secondary variables to Python BC

Don't review yet! Soon: review commit-wise.

@chleh chleh added the WIP 🏗 label Jul 31, 2018

@endJunction

This comment has been minimized.

Copy link
Member

endJunction commented Aug 2, 2018

I'm afraid to ask, but 132kloc?

@chleh

This comment has been minimized.

Copy link
Collaborator Author

chleh commented Aug 2, 2018

Yes. I copied OGS, and Python (2 and 3), and combined the two, and now there are Python BCs. 🤣
No. I assume its due to WIP ParaView state files. I'll remove them. Maybe I'll keep some of them to simplify the validation of (changed) reference solutions.

@chleh chleh force-pushed the chleh:python-bcs-for-pr branch 3 times, most recently from 2cced46 to ca03670 Aug 7, 2018

# snprintf might be defined even though it's not needed. Precompiled headers
# cause that this macro definition is visible in all of ProcessLib.
# That might lead to clashes with jsoncpp. In particular the following
# compilation error was observed on MSVC:

This comment has been minimized.

@endJunction

endJunction Aug 8, 2018

Member

I could imagine that msvc version and python version would be helpful...

This comment has been minimized.

@chleh

chleh Aug 15, 2018

Author Collaborator

Issue solved. Code and comment removed.

@chleh chleh force-pushed the chleh:python-bcs-for-pr branch from 1a1e493 to f0ca87c Aug 8, 2018

"script.");
return;
}
if (val.first)

This comment has been minimized.

@endJunction

endJunction Aug 13, 2018

Member

if (!val,first) return

// gather primary variable values
primary_variables.clear();
auto const num_var = _dof_table_boundary->getNumberOfVariables();
for (int var = 0; var < num_var; ++var)

This comment has been minimized.

@endJunction

endJunction Aug 13, 2018

Member

check for HM


add_library(ogs_python_bindings STATIC ogs_python_bindings.cpp)
target_link_libraries(ogs_python_bindings PRIVATE pybind11::embed)
target_include_directories(ogs_python_bindings PRIVATE ${PYTHON_INCLUDE_DIRS})

This comment has been minimized.

@endJunction

endJunction Aug 13, 2018

Member

necessary?

This comment has been minimized.

@chleh

chleh Aug 13, 2018

Author Collaborator

Cleaned up.

target_link_libraries(ogs PRIVATE pybind11::embed)

add_library(ogs_python_bindings STATIC ogs_python_bindings.cpp)
target_link_libraries(ogs_python_bindings PRIVATE pybind11::embed)

This comment has been minimized.

@endJunction

endJunction Aug 13, 2018

Member

Consider PUBLIC.

This comment has been minimized.

@chleh

chleh Aug 13, 2018

Author Collaborator

Cleaned up.

@chleh chleh force-pushed the chleh:python-bcs-for-pr branch from 9aaea6a to f925e71 Aug 13, 2018

// OpenGeoSys Python module. The name is generated by pybind11. If it is not
// obvious that this symbol is actually used, the linker might remove it
// under certain circumstances.
mark_used(&pybind11_init_impl_OpenGeoSys);

This comment has been minimized.

@endJunction

endJunction Aug 14, 2018

Member

just an idea...
Maybe declaring the pybind11_init_impl_OpenGeoSys function with __attribute__((used)) helps (at least for gcc and clang).

This comment has been minimized.

@chleh

chleh Aug 15, 2018

Author Collaborator

__attribute__(used) is gcc-specific. I didn't find the same for MSVC:
https://msdn.microsoft.com/de-de/library/74f4886t.aspx

Maybe __declspec(dllexport) might help?
https://msdn.microsoft.com/de-de/library/3y1sfaz2.aspx
But is that also applicable for static libs?

Anyway, we'd have to distinguish between different compilers, which is also kind of hackish...


for (int i = 0; i < getNumberOfComponents(); ++i)
{
global_component_ids.push_back(i);

This comment has been minimized.

@endJunction

endJunction Aug 14, 2018

Member

std::iota(begin(global_component_ids), end(global_component_ids), getNumberOfComponents())
Ups...

A "slightly" verbose version would be something like

std::generate_n(std::back_inserter(global_component_ids), getNumberOfComponents(),
                [n = 0] () mutable { return n++; });

Nevermind

This comment has been minimized.

@chleh

chleh Aug 14, 2018

Author Collaborator

Indeed, verbose 😄

}

std::unique_ptr<LocalToGlobalIndexMap>
LocalToGlobalIndexMap::deriveBoundaryConstrainedMap(

This comment has been minimized.

@endJunction

endJunction Aug 14, 2018

Member

Maybe the two versions of deriveBCMap could be merged into one?

This comment has been minimized.

@chleh

chleh Aug 15, 2018

Author Collaborator

I'll open an issue.

"bulk_node_ids");

// gather primary variables
Eigen::MatrixXd primary_variables_mat(num_nodes, num_comp_total);

This comment has been minimized.

@endJunction

endJunction Aug 14, 2018

Member

Optionally use compile time size for nodes' size...

This comment has been minimized.

@chleh

chleh Aug 14, 2018

Author Collaborator

Hm... I think, I'd like to leave it all dynamic:
Eigen::Matrix<double, ShapeFunction::size /*?*/, Eigen::Dynamic> doesn't really look more pleasant.

# self.GetOutputDataObject(0).GetPointData().AddArray(r2)
# self.GetOutputDataObject(0).GetPointData().AddArray(c2)

# self.GetOutputDataObject(0).SetPoints(pts) #.SetData(c2)

This comment has been minimized.

@endJunction

endJunction Aug 14, 2018

Member

Maybe some debug comments could be removed...

This comment has been minimized.

@chleh

chleh Aug 15, 2018

Author Collaborator

removed by now.

@endJunction
Copy link
Member

endJunction left a comment

Maybe you could open few issues for the remaining modifications after/close to the merge.

Especially one for the deriveBCMap unification...

@wenqing
Copy link
Member

wenqing left a comment

👍

for (unsigned element_node_id = 0; element_node_id < num_nodes;
++element_node_id)
{
auto* node = _element.getNode(element_node_id);

This comment has been minimized.

@wenqing

wenqing Aug 14, 2018

Member

may be auto const* const node

This comment has been minimized.

@chleh

chleh Aug 15, 2018

Author Collaborator

changed.

_data.dof_table_bulk.getGlobalIndex(loc, var, comp);
if (dof_idx == NumLib::MeshComponentMap::nop)
{
// TODO extend Python BC to mixed FEM

This comment has been minimized.

@wenqing

wenqing Aug 14, 2018

Member

So far we do not have mixed FEM implemented. Maybe it can be said as "extend Python BC to processes using Taylor-Hood elements".

This comment has been minimized.

@chleh

chleh Aug 15, 2018

Author Collaborator

// TODO extend Python BC to mixed FEM ansatz functions still fits on a single line (and therefore is displayed completely in QtCreator's todo list). I'd suggest that wording.

values in every load step.
Due to symmetry reasons a flat circular contact area of radius $a$ forms.

{{< img src="../hertz-contact.pnK" >}}

This comment has been minimized.

@wenqing

This comment has been minimized.

@chleh

chleh Aug 15, 2018

Author Collaborator

The file names now are fixed.

@bilke

This comment has been minimized.

Copy link
Member

bilke commented Aug 14, 2018

Regarding VTK Python version mismatch:

In your VTK installation there is a file lib/cmake/vtk-8.1/VTKConfig.cmake. Can you please check if there is some info about Python version. If yes (e.g. in the variable VTK_PYTHON_VERSION) you can write a check in our CMake similar to this:

if (NOT ${VTK_PYTHON_VERSION} VERSION_EQUAL ${PYTHON_VERSION_STRING})
...
endif()
if (!_flush)
return;

using namespace pybind11::literals;

This comment has been minimized.

@TomFischer

TomFischer Aug 15, 2018

Member

I'm not familiar with pybind11 and I wonder why the namespace here is needed?

This comment has been minimized.

@chleh

chleh Aug 15, 2018

Author Collaborator

For the line below, e.g. "end"_a is the equivalent to a Python named argument.

return;

using namespace pybind11::literals;
pybind11::print("end"_a = "", "flush"_a = true);

This comment has been minimized.

@TomFischer

TomFischer Aug 15, 2018

Member

Could you explain this line, please?

This comment has been minimized.

@chleh

chleh Aug 15, 2018

Author Collaborator

It's equivalent to (Python) print(end="", flush=True), i.e., no end-of-line will be printed and output will be flushed.
So the behaviour is described in the docu of the destructor. Is that sufficient?

This comment has been minimized.

@TomFischer

TomFischer Aug 15, 2018

Member

Thanks for the explanation. Is my understanding correct that _a means that the previous characters is a python argument name?

This comment has been minimized.

@chleh

chleh Aug 15, 2018

Author Collaborator

Yes. _a is a user-defined literal.
There are also other such literals, e.g. s in C++14.
E.g. in

using namespace std::string_literals;
auto str = "test"s;

str is a std::string, not a character array.

@bilke

bilke approved these changes Aug 15, 2018

Copy link
Member

bilke left a comment

There is still a ParaView state file in this PR. Is it intended to keep? Other than that, very good work!

PUBLIC BaseLib MathLib MeshLib NumLib logog)

target_link_libraries(ProcessLibBoundaryConditionPython
PRIVATE pybind11::pybind11)

This comment has been minimized.

@bilke

bilke Aug 15, 2018

Member

You can merge this command with the previous one:

target_link_libraries(ProcessLibBoundaryConditionPython
    PUBLIC BaseLib MathLib MeshLib NumLib logog
    PRIVATE pybind11::pybind11)

This comment has been minimized.

@chleh

chleh Aug 15, 2018

Author Collaborator

All pvsm files will be deleted.
Thanks for the hint. I'll change it.

MeshLib::MeshItemType::Node,
bulk_node_id};
auto const dof_idx =
_bc_data.dof_table_bulk.getGlobalIndex(loc, var, comp);

This comment has been minimized.

@TomFischer

TomFischer Aug 15, 2018

Member

Is it possible to compute the dof_idx also from the boundary dof table?

This comment has been minimized.

@chleh

chleh Aug 15, 2018

Author Collaborator

I think, it's not. I think I tried, but the dof table for the boundary is for a hypothetical equation system that is assembled solely on the boundary (i.e., with no connection to the bulk). Therefore all boundary dof indices would be wrong. @endJunction, is that right? If unsure, I can also check that.

a = np.empty(coords.shape[0])

for i, (x, y, z) in enumerate(coords):
r = np.sqrt(x**2 + y**2)

This comment has been minimized.

@TomFischer

TomFischer Aug 15, 2018

Member

Where is r used?

This comment has been minimized.

@chleh

chleh Aug 15, 2018

Author Collaborator

In some old version of this script, long, long time ago. I'll remove it.

@TomFischer
Copy link
Member

TomFischer left a comment

Looks good. 👍

@ufz ufz deleted a comment from bilke Aug 15, 2018

@ufz ufz deleted a comment from bilke Aug 15, 2018

@ufz ufz deleted a comment from bilke Aug 15, 2018

@chleh chleh force-pushed the chleh:python-bcs-for-pr branch from bf64d94 to 7007188 Aug 17, 2018

@chleh chleh referenced this pull request Aug 17, 2018

Merged

Python BCs: Hertz Contact test #2184

1 of 1 task complete
@chleh

This comment has been minimized.

Copy link
Collaborator Author

chleh commented Aug 17, 2018

I just rebased and squashed this PR. Thank you for the quick reviewing process and for your help.
Lately, netlify failed because hugo could not resolve some page reference. Locally, at my machine it works, however. @bilke, could you please have a look.

Unfortunately I had to exclude the Hertz contact test, because it requires a non-trivial change to the non-linear solver, cf. #2184.

Remaining for this PR are:

  • I decided not to use __attribute__(used) because it's not portable and I don't know of a version that works for MSVC.
  • The Elder test case is a LARGE one. Are LARGE tests tested with a Python build?

If this PR builds successfully, from my perspective it's ready to be merged if you agree.

@endJunction

This comment has been minimized.

Copy link
Member

endJunction commented Aug 17, 2018

LARGE tests are not tested.

Christoph Lehmann added some commits Aug 17, 2018

@chleh chleh force-pushed the chleh:python-bcs-for-pr branch from cd5dbf1 to 7eda63e Aug 17, 2018

@bilke

This comment has been minimized.

Copy link
Member

bilke commented Aug 17, 2018

For the failing web build merge this: chleh#10

Merge pull request #10 from bilke/fix-netlify
[web] Updated Hugo to 0.47.

@chleh chleh referenced this pull request Aug 17, 2018

Open

Clean up DOF table #2186

@chleh chleh removed the WIP 🏗 label Aug 17, 2018

@endJunction endJunction merged commit 3b1cb39 into ufz:master Aug 17, 2018

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

@chleh chleh deleted the chleh:python-bcs-for-pr branch Aug 18, 2018

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.