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

Faster nodal patch recovery #18721

Merged
merged 4 commits into from
Dec 10, 2021
Merged

Faster nodal patch recovery #18721

merged 4 commits into from
Dec 10, 2021

Conversation

hugary1995
Copy link
Contributor

@hugary1995 hugary1995 commented Aug 28, 2021

  • Enable nodal patch recovery in parallel using RMs
  • Refactor to avoid redundant material property evaluations

refs #15748
closes #12036

@hugary1995
Copy link
Contributor Author

@lynnmunday I remember you are interested in this as well.

@dschwen
Copy link
Member

dschwen commented Aug 28, 2021

You did it!

@hugary1995
Copy link
Contributor Author

I did a rough performance comparison by running the test with a finer mesh and more time steps:

The new nodal patch recovery:

  • The node loop 0.149 seconds
  • The element loop 1.173 seconds

Using first order monomial:

  • The element loop 1.015 seconds

Using constant monomial:

  • The element loop 1.035 seconds

I think this new nodal patch recovery is now actually "usable".

@hugary1995
Copy link
Contributor Author

Accuracy comparison (just by visualizing, I don't need to prove that NPR is super-convergent):

Nodal patch recovery
stress_xx_npr

First order monomial
stress_xx_nodal

Constant monomial
stress_xx_elemental

@hugary1995
Copy link
Contributor Author

I have to say that, if all you need is visualization, using the CellDataToPointData filter in paraview is probably the best choice:
stress_xx_paraview

As you can see, the result is very similar to what was obtained with nodal patch recovery.

But if you need gradients of quantities defined at quadrature points during the simulation, nodal patch recovery is probably the way to go.

@hugary1995
Copy link
Contributor Author

You did it!

Yeah, this is implementing exactly what we discussed yesterday. The not-so-good part of this design is that we need to write a new UserObject to replace each Auxkernel, e.g. I wrote NPRRankTwoAux just to replace RankTwoAux. But I guess for tensor_mechanics, having several commonly UserObjects is probably enough.

@moosebuild
Copy link
Contributor

moosebuild commented Aug 28, 2021

Job Documentation on 3901256 wanted to post the following:

View the site here

This comment will be updated on new commits.

@dschwen
Copy link
Member

dschwen commented Aug 28, 2021

IndexableProperty which I propose in #18661 should allow you to write one user object that can handle reals, vectors, and all rank tensors we have in one single object.

@dschwen
Copy link
Member

dschwen commented Aug 30, 2021

Can you revert the change from getActiveLocalElementRange to getEvaluableElementRange and manually run a loop over all elements that are evaluable but not local?

@hugary1995
Copy link
Contributor Author

Can you revert the change from getActiveLocalElementRange to getEvaluableElementRange and manually run a loop over all elements that are evaluable but not local?

Yes, I think I get it now. See my reply to Alex above.

@dschwen
Copy link
Member

dschwen commented Aug 30, 2021

Alternatively you can do point to point communication in the user object!

@dschwen
Copy link
Member

dschwen commented Aug 30, 2021

@lindsayad
Copy link
Member

@dschwen that code looks like a very good candidate for the push/pull family of functions in TIMPI. I think it could probably reduce the LOC by quite a bit. Yes @hugary1995 if you need nonlocal data pull_parallel_vector_data is going to be a good thing to look into. pull instead of push because an owning process does not know who needs its data. But the puller knows who owns the data it needs and so then can query and "pull" from the owner

@dschwen
Copy link
Member

dschwen commented Aug 30, 2021

@dschwen that code looks like a very good candidate for the push/pull family of functions in TIMPI. I think it could probably reduce the LOC by quite a bit.

Thanks for the pointer! (That code is a little over 3 years old - older than TIMPI :-D. But I should definitely check out the push/pull stuff!)

@hugary1995 hugary1995 marked this pull request as ready for review August 31, 2021 15:37
@hugary1995
Copy link
Contributor Author

There's an external app failing (probably due to the algebraic ghosting fix). Is it my responsibility to fix/regold the test in that app?

@lindsayad
Copy link
Member

There's an external app failing (probably due to the algebraic ghosting fix). Is it my responsibility to fix/regold the test in that app?

Nah. I'll handle it

large_media Outdated Show resolved Hide resolved
@moosebuild
Copy link
Contributor

Job Precheck on 6d700bb wanted to post the following:

A change of the following file(s) triggered this check:

libmesh

The following file(s) are unchanged:

conda/mpich/conda_build_config.yaml
conda/libmesh/meta.yaml

The libmesh submodule or configuration was changed but the conda build config was not

@hugary1995
Copy link
Contributor Author

hugary1995 commented Dec 3, 2021

@dschwen with #19000 #19500 can I use IndexableProperty now?

@dschwen
Copy link
Member

dschwen commented Dec 3, 2021

Yes!

@hugary1995
Copy link
Contributor Author

A warm greeting when you wake up tomorrow: The IndexableProperty is giving me the same error

test:nodal_patch_recovery.nodal_patch_recovery: *** ERROR ***
test:nodal_patch_recovery.nodal_patch_recovery: The following error occurred in the object "stress_xx_patch", of type "NodalPatchRecoveryMaterialProperty".
test:nodal_patch_recovery.nodal_patch_recovery: 
test:nodal_patch_recovery.nodal_patch_recovery: The non-AD material property 'stress' does not exist

If I do not check the validity of the indexable property at initialSetup, then it solves the first step then I get the following error:

test:nodal_patch_recovery.nodal_patch_recovery: Time Step 1, time = 0.05, dt = 0.05
test:nodal_patch_recovery.nodal_patch_recovery: 
test:nodal_patch_recovery.nodal_patch_recovery: Performing automatic scaling calculation
test:nodal_patch_recovery.nodal_patch_recovery: 
test:nodal_patch_recovery.nodal_patch_recovery:  0 Nonlinear |R| = 1.118034e-03
test:nodal_patch_recovery.nodal_patch_recovery:  1 Nonlinear |R| = 9.428704e-08
test:nodal_patch_recovery.nodal_patch_recovery:  2 Nonlinear |R| = 2.247769e-14
test:nodal_patch_recovery.nodal_patch_recovery:  Solve Converged!
test:nodal_patch_recovery.nodal_patch_recovery: 
test:nodal_patch_recovery.nodal_patch_recovery: 
test:nodal_patch_recovery.nodal_patch_recovery: *** ERROR ***
test:nodal_patch_recovery.nodal_patch_recovery: The following error occurred in the object "stress_xx_patch", of type "NodalPatchRecoveryMaterialProperty".
test:nodal_patch_recovery.nodal_patch_recovery: 
test:nodal_patch_recovery.nodal_patch_recovery: internal error in IndexableProperty

@dschwen
Copy link
Member

dschwen commented Dec 3, 2021 via email

@dschwen
Copy link
Member

dschwen commented Dec 3, 2021

D'Oh, ok, I get it. That's a design flaw that I need to think about a bit.

I have getOptionalMaterialProperty methods in Material.h and MaterialPropertyInterface.h. The methods in Material do the late binding, the methods in MaterialPropertyInterface are for objects that run later when all materials exist.

This of course does not take care of objects that run entirely before the first material is even constructed.

@loganharbour that's why I actually wanted to store the proxies in FEProblem. We'll have to come up with a better design that takes care of that 3rd phase of objects that are constructed even earlier than materials.

I should have thought of the UserObject case! My tests for that PR only use Material and AuxKernel. Dang!

@dschwen
Copy link
Member

dschwen commented Dec 6, 2021

#19546 will fix this

hugary1995 and others added 3 commits December 7, 2021 21:27
- Enable nodal patch recovery in parallel using RMs
- Refactor to avoid redundant material property evaluations
- Use the modern push/pull during finalize() of NodalPatchRecoveryBase to
communicate precomputed elemental least squares coefficients and values.

refs idaholab#15748
closes idaholab#12036

Co-authored-by: Alex Lindsay <alexlindsay239@gmail.com>
@hugary1995
Copy link
Contributor Author

Writing this PR has been fun. New capabilities like the parallel push/pull interfaces and IndexableProperty are making my life so much easier as a downstream developer.

dschwen
dschwen previously approved these changes Dec 9, 2021
Copy link
Member

@dschwen dschwen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me.

@dschwen dschwen merged commit a7794ad into idaholab:next Dec 10, 2021
@bwspenc
Copy link
Contributor

bwspenc commented Dec 15, 2021

I'm really excited to have this finally merged in!

One minor follow-on -- this broke the documentation builds for the apps that do sqa checks because that test points to nodal_patch_recovery.md, which is not in tensor_mechanics. We've had to go back and specifically add this file in to the config files for the apps that depend on tensor_mechanics. We really should fix this by making that test point to something that's already included by the apps.

@bwspenc
Copy link
Contributor

bwspenc commented Dec 15, 2021

For example, see this issue: idaholab/blackbear#285

@hugary1995
Copy link
Contributor Author

So in principal the tests in a module should only point to docs in that module (or dependent modules). Otherwise, apps that depend on that module will have to explicitly include those docs in the config. Am I understanding it correctly?

I guess we can either add - modules/doc/content to the tensor mechanics doc config file, or move modules/doc/content/help/finite_element_concepts/nodal_patch_recovery.md to somewhere in the framework/doc/content

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stateful Material Properties don't play nicely with NodalPatchRecovery
9 participants