cb-geo / mpm

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

[Feature] State variable vtk output for different phases #683

Closed bodhinandach closed 3 years ago

bodhinandach commented 3 years ago

Describe the PR This PR enhances the capability to output vtk files for two-phase particles following the material container refactoring done in PR #675 and #678.

Related Issues/PRs PR #675 and #678 as well as PR #680 and Issue #633.

Additional context This will break the existing input configuration. User should change the current vtk_statevars arrangement:

"vtk_statevars": ["pressure", "pdstrain"]

to the new one with phase_id:

"vtk_statevars": [
    {
        "phase_id" : 0,
        "statevars": ["pressure", "pdstrain"]
    },
    {
        "phase_id" : 1,
        "statevars": ["pressure"]
    }
]

If the user doesn't specify the phase_id, it is assumed that the phase_id is 0 by default.

"vtk_statevars": [
    {
        "statevars": ["pressure", "pdstrain"]
    }
]

There is a slight change also in the outputted file name. Instead of producing pressureXYZ.vtp or pressureXYZ.pvtp as output, the modified code will produce P0pressureXYZ.vtp and P0pressureXYZ.pvtp or P1pressureXYZ.vtp and P1pressureXYZ.pvtp for the solid and liquid phase, respectively. I am okay with any other idea of naming the output file, though we need to make it different, as otherwise, the second phase will overwrite the first one.

codecov[bot] commented 3 years ago

Codecov Report

Merging #683 into develop will increase coverage by 0.02%. The diff coverage is 77.14%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #683      +/-   ##
===========================================
+ Coverage    96.66%   96.68%   +0.02%     
===========================================
  Files          123      123              
  Lines        25375    25387      +12     
===========================================
+ Hits         24527    24543      +16     
+ Misses         848      844       -4     
Impacted Files Coverage Δ
include/mesh.h 100.00% <ø> (ø)
include/solvers/mpm_base.h 0.00% <ø> (ø)
tests/io/write_mesh_particles.cc 87.27% <0.00%> (-0.40%) :arrow_down:
tests/io/write_mesh_particles_unitcell.cc 87.68% <0.00%> (-0.42%) :arrow_down:
include/solvers/mpm_base.tcc 75.12% <79.31%> (+1.43%) :arrow_up:
include/mesh.tcc 83.90% <100.00%> (ø)

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 3b20da5...6b6c735. Read the comment docs.

kks32 commented 3 years ago

We can use the updated alternative, no reason for us to be backward compatible in this case.

"vtk_statevars": [
    {
        "phase_id" : 0,
        "statevars": ["pressure", "pdstrain"]
    },
    {
        "phase_id" : 1,
        "statevars": ["pressure"]
    }
]
bodhinandach commented 3 years ago

It means then we need to modify all mpm.json, are you ok with that? Can we at least make it deprecated for some time and output a deprecated message to let the user change their format?

kks32 commented 3 years ago

Yes, otherwise it's too much of unnecessary branching conditions. No, we create a breaking change, no need for deprecated message or sometime to change. If they use the old format, the code will throw an error, that should be sufficient.

bodhinandach commented 3 years ago

@kks32 I added the breaking change. Ping @cb-geo/mpm for info. I will make a new pull request to the benchmark repo to change the input json there as soon as this is merged.