zhang-alvin commented 4 years ago

Mandatory Checklist

Please ensure that the following criteria are met:

[ ] Title of pull request describes the changes/features
[ ] Request at least 2 reviewers
[ ] If new files are being added, the files are no larger than 100kB. Post the file sizes.
[ ] Code coverage did not decrease. If this is a bug fix, a test should cover that bug fix. If a new feature is added, a test should be made to cover that feature.
[x] New features have appropriate documentation strings (readable by sphinx)
[x] Contributor has read and agreed with CONTRIBUTING.md and has added themselves to CONTRIBUTORS.md

As a general rule of thumb, try to follow PEP8 guidelines.

Description

Addresses #1208

https://github.com/erdc/proteus/commit/8e7c357baa9ed44d11fb404415bc6f7950dd6abe introduced an error in the postStep for MoveMesh in parallel. Specifically, the global mesh object was being updated instead of the subdomain mesh object, leading to a bookkeeping error.

A parallel test command is added to Travis in the FSI directory for testing/covering this behavior.

zhang-alvin commented 4 years ago

The parallel test does not pass locally for me.

mpirun -np 2 pytest test_FSI.py -k "floatingCylinder"
============================= test session starts ==============================
platform linux -- Python 3.7.4, pytest-3.4.1, py-1.5.2, pluggy-0.6.0
rootdir: /home/zhanga/barn/proteus/temp_proteusRpath, inifile:
plugins: xdist-1.22.2, forked-0.2, cov-2.5.1
============================= test session starts ==============================
platform linux -- Python 3.7.4, pytest-3.4.1, py-1.5.2, pluggy-0.6.0
rootdir: /home/zhanga/barn/proteus/temp_proteusRpath, inifile:
plugins: xdist-1.22.2, forked-0.2, cov-2.5.1
collected 4 items

test_FSI.py collected 4 items

test_FSI.py F                                                            [100%]
...
[       5]    Newton it 1 == maxIts FAILED convergenceTest = r
[       5]    Newton it 1 norm(r) =  2.89750e-04         norm(r)/(rtol*norm(r0)+atol) =  1.15900e+03
[       5] Step failed due to solver failure
[       5] Sequence step failed
[       5] Sequence failed
[       5] System Step Failed
...
=================================== FAILURES ===================================
_______________________ TestIBM.test_floatingCylinderALE _______________________
        ns = NumericalSolution.NS_base(so,pList,nList,so.sList,opts)
        ns.calculateSolution('floatingCylinderALE')
        pos = case.body.getPosition()

>       npt.assert_almost_equal(pos, np.array([0.5, 0.5074055958, 0.]), decimal=5)
E       AssertionError: 
E       Arrays are not almost equal to 5 decimals
E       
E       Mismatched elements: 1 / 3 (33.3%)
E       Max absolute difference: 0.
E       Max relative difference: 0.01
E        x: array([0.5 , 0.51, 0.  ])
E        y: array([0.5    , 0.50741, 0.     ])

test_FSI.py:189: AssertionError

cekees commented 4 years ago

Which model's Newton iteration is failing?

zhang-alvin commented 4 years ago

Which model's Newton iteration is failing?

AddedMass

cekees commented 4 years ago

Have you tried it by commenting out those new postStep lines in MoveMesh.py instead of trying to fix them (just in case the fix is not really a fix)? It runs for me with them commented out. There is also another issue that needs fixing, which is that in the TwoPhaseFlow app Parameters.py, we need to force the AddedMass PETSc configuration to always use the iterative solver configuration. The superlu/superlu_dist configuration seems to work for me reliably, but since the system is singular (constant null space), I think we generally need to be using an iterative solver configured to be robust and scalable for pure Neumann Poisson problems. That doesn't explain why your commit fails and my local changes succeed though, because I haven't made that change yet.

zhang-alvin commented 4 years ago

Have you tried it by commenting out those new postStep lines in MoveMesh.py instead of trying to fix them (just in case the fix is not really a fix)? It runs for me with them commented out. There is also another issue that needs fixing, which is that in the TwoPhaseFlow app Parameters.py, we need to force the AddedMass PETSc configuration to always use the iterative solver configuration. The superlu/superlu_dist configuration seems to work for me reliably, but since the system is singular (constant null space), I think we generally need to be using an iterative solver configured to be robust and scalable for pure Neumann Poisson problems. That doesn't explain why your commit fails and my local changes succeed though, because I haven't made that change yet.

Yes commenting out those lines did allow the tests to pass. Maybe for this test the cylinder isn't moving enough for the postStep corrections to matter so much.

I can achieve a similar effect as modifying the AddedMass PETSc configuration for this test case by directly supplying a petsc configuration file right?

zhang-alvin commented 4 years ago

To recap, those lines were added because the node/elementDiameters needed to be updated at each time-step. There might be a less intrusive way of getting those updated.

codecov[bot] commented 4 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@8067b87). Click here to learn what that means. The diff coverage is 66.66%.

@@            Coverage Diff            @@
##             master    #1211   +/-   ##
=========================================
  Coverage          ?   50.87%           
=========================================
  Files             ?      526           
  Lines             ?   108803           
  Branches          ?        0           
=========================================
  Hits              ?    55357           
  Misses            ?    53446           
  Partials          ?        0

Impacted Files	Coverage Δ
proteus/mprans/MoveMesh.py	`86.21% <ø> (ø)`
proteus/tests/FSI/test_FSI.py	`47.28% <66.66%> (ø)`
...linder2D/conforming_rans2p/symmetricDomain_john.py	`95.42% <0.00%> (ø)`
..._tension/rising_bubble_rans3p/pressureInitial_n.py	`92.85% <0.00%> (ø)`
...tests/BernsteinPolynomials/poisson_eqn/L2_batch.py	`0.00% <0.00%> (ø)`
proteus/tests/HotStart_3P/twp_navier_stokes_n.py	`80.00% <0.00%> (ø)`
...solver_tests/import_modules/twp_navier_stokes_p.py	`0.00% <0.00%> (ø)`
.../tests/matrix_constructor/test_mass_matrix_quad.py	`94.59% <0.00%> (ø)`
...tests/cylinder2D/ibm_method/pressureincrement_p.py	`0.00% <0.00%> (ø)`
proteus/tests/TwoPhaseFlow/test_TwoPhaseFlow.py	`85.48% <0.00%> (ø)`
... and 518 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 8067b87...aafce45. Read the comment docs.

zhang-alvin commented 4 years ago

It looks like the Petsc persistent options issue is causing the lingering Travis failure. I can run the tests sucessfully in the FSI directory, but it's failing when run as a suite.

zhang-alvin commented 4 years ago

Scratch that, misinterpretation. It's the serial test that's not yielding the proper values.

cekees commented 4 years ago

You may have to tighten the nonlinear and linear tolerance to get 5 decimal places of accuracy given that the problem is actually nonlinear and there will be some variation in the result of the linear solve. It looks to me like the test doesn't set the tolerances so they are defaulting to values relative to the characteristic cell diameter, he, which is presumably pretty large for this quick test.

zhang-alvin commented 4 years ago

You may have to tighten the nonlinear and linear tolerance to get 5 decimal places of accuracy given that the problem is actually nonlinear and there will be some variation in the result of the linear solve. It looks to me like the test doesn't set the tolerances so they are defaulting to values relative to the characteristic cell diameter, he, which is presumably pretty large for this quick test.

Is there a setting akin to a direct solve like with use superLU in serial? It looks like it's using the superlu_dist petsc options so I'm a little surprised that there would be any differences from the linear algebra.

zhang-alvin commented 4 years ago

I think this might take a little longer to diagnose the issue. I'm not convinced that the values should deviate on the order of 1e-5 in parallel if it's using superlu_dist as the linear solver. I'm also not convinced that the mesh coordinates are being reliably updated in parallel.

Just for the sake of getting the tutorials test in place and a parallel test in place, I suggest rolling back the node/element diameter correction as it hasn't led to any noticeable errors and then reopening the corresponding issue. I can take a deeper look at this when I'm actively looking at a relevant case (PUMI falling cylinder case).

cekees commented 4 years ago

OK, maybe try rolling it back on this PR and check if the new parallel tests pass.

cekees commented 4 years ago

Also, to answer your question, use useSuperlu=True automatically does superlu in serial and superlu_dist in parallel.

erdc / proteus

Fix MoveMesh cases #1211

Mandatory Checklist

Description

Codecov Report