cb-geo / mpm

CB-Geo High-Performance Material Point Method
https://www.cb-geo.com/research/mpm
Other
238 stars 83 forks source link

Add return value for nodal friction and velocity constraints #667

Closed ezrayst closed 4 years ago

ezrayst commented 4 years ago

Describe the PR The current assign nodal constraints in mpm_base for both velocity and friction do not throw when the bool returns false. Therefore, this PR returns the bool and throw if false.

codecov[bot] commented 4 years ago

Codecov Report

Merging #667 into develop will decrease coverage by 0.02%. The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #667      +/-   ##
===========================================
- Coverage    96.56%   96.54%   -0.02%     
===========================================
  Files          122      122              
  Lines        25265    25269       +4     
===========================================
  Hits         24395    24395              
- Misses         870      874       +4     
Impacted Files Coverage Δ
include/solvers/mpm_base.tcc 69.43% <0.00%> (-0.49%) :arrow_down:

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 a03af6c...eda34ba. Read the comment docs.

kks32 commented 4 years ago

@ezrayst Thanks. Could you please test the exception: https://github.com/catchorg/Catch2/blob/master/docs/assertions.md#exceptions

ezrayst commented 4 years ago

I want to discuss one more thing regarding this, let's use velocity constraints as example:

I do not think we need to check for NOTHROW for nodal_velocity_constraints in mpm_base anymore. I would leave it as is.

Further, this calls to change this function to a void instead of bool. What do you think?

kks32 commented 4 years ago

I want to discuss one more thing regarding this, let's use velocity constraints as example:

* `mpm_explicit` calls `bool` `initialise_mesh()` ([link](https://github.com/cb-geo/mpm/blob/a03af6cc5afa14c1b9e832b741e78a62eac89e46/include/solvers/mpm_base.tcc#L115))

* which in `mpm_base` calls `void` `nodal_velocity_constraints` ([link](https://github.com/cb-geo/mpm/blob/a03af6cc5afa14c1b9e832b741e78a62eac89e46/include/solvers/mpm_base.tcc#L182)) (I think the idea is to check for `NOTHROW` here right?)

* which in `constraints` calls `assign_nodal_velocity_constraint` ([link](https://github.com/cb-geo/mpm/blob/9ed027de4fb58f4edf5866b1f25d08a4fbae887a/include/loads_bcs/constraints.tcc#L3)). This function already has a throw that is tested.

I do not think we need to check for NOTHROW for nodal_velocity_constraints in mpm_base anymore. I would leave it as is.

Further, this calls to change this function to a void instead of bool. What do you think?

If we want to address this issue, these functions should throw and abort on MPM main. There is no reason to continue when things fail.