idaholab / moose

Multiphysics Object Oriented Simulation Environment
https://www.mooseframework.org
GNU Lesser General Public License v2.1
1.77k stars 1.05k forks source link

Runtime error when using two materials in a domain #16992

Open jain651 opened 3 years ago

jain651 commented 3 years ago

Bug Description

Runtime error while studying a domain two materials:

Steps to Reproduce

Use input file in tensor_mechanics/test/tests/two_materials_test/two_element_model.i

Impact

Could not study creep in concrete with soil in the foundation. This is the thing that is observed, however, it may impact other problems too.

dschwen commented 3 years ago

It would be clearer if you used mesh generators. We'll have to look at the exodus file to understand what the mesh looks like now.

dschwen commented 3 years ago

First therory is that a material property cannot be stateful on only part of the domain. @aeslaughter is that the case?

dschwen commented 3 years ago

All mc* user objects in your example cannot be block restricted.

dschwen commented 3 years ago

So if I add an empty initQpStatefulProperties() method to ComputeElasticityTensorTempl and remove the invalid block restrictions, the example runs.

jain651 commented 3 years ago

It would be clearer if you used mesh generators. We'll have to look at the exodus file to understand what the mesh looks like now.

I have added an example with using meshgenerator

dschwen commented 3 years ago

I suppose "fix" is an overstatement. It rather is a workaround hack.

jain651 commented 3 years ago

I guess I need to do similar changes if I use ComputeIsotropicElasticityTensor. Should I submit a PR for these changes?

dschwen commented 3 years ago

I don't think it is the right thing to do. I'd like to hear from @aeslaughter or @lindsayad to see if this is really a limitation of the material storage system, and if it could be changed without too much effort.

aeslaughter commented 3 years ago

I do think that you need to have a stateful property everywhere, but I am not sure. @rwcarlsen or @andrsd might actually know.

lindsayad commented 3 years ago

So the core issue is that a material property cannot be stateful on just part of the domain? If so we should clean up the issue title. Also if that is the core issue, I would be a bit surprised that this is our first documentation of it.

I don't think this would be fast to fix. Our MaterialPropertyStorage containers really have no concept of SubdomainIDs. It's basically if a property is stateful (anywhere) then we try to swap back and forth between MaterialData and MaterialPropertyStorage

bwspenc commented 3 years ago

I just stumbled on this, and made the same hack that Daniel had made to work around this. I was just going to make an issue, but found this one.

I was initially a little surprised that we hadn't hit this more often with our mechanics models, because it seems like this would come up a lot for people using multiple material models. I think I hasn't come up much because the base classes for the stress and strain calculators appear to already mostly override initQpStatefulProperties(). It's pretty unusual for us to make the elasticity tensor stateful, as is being done in this model.

So is it the case that if a property is made stateful for one block, it's made stateful everywhere? That's what it kind of looked like in my brief search of the code. I agree with @lindsayad that this would not be a quick fix. Is there something we can do to get around this issue in the mean time? We can hack around this by either overriding initQpStatefulProperties() or commenting out that error message, but it would be nice to be able to run these models without modifying the code.

rwcarlsen commented 3 years ago

Yeah - this is just a false-positive on triggering an error because we don't track on which blocks specifically a material property is requested to be stateful. We would also need to map/track the materialprop+block --> materialobject associations.

bwspenc commented 3 years ago

I spent a little time digging around that code, and it looks to me like even adding those associations is nontrivial -- like it would almost be just as much work to really do this right and restrict the stateful properties by block.

rwcarlsen commented 3 years ago

Yeah - it would be tricky. Fraught with corner cases and gotchas.

bwspenc commented 3 years ago

Should we just remove that overzealous error?

bwspenc commented 3 years ago

The easy "fixes" are to implement empty initQpStatefulProperties() methods for the affected materials or to remove that error. I would vote for the latter, since it is preventing us from running a perfectly valid class of models.

lindsayad commented 3 years ago

It would be great if we got the actual error message printed on this ticket so we don't all have to go one-by-one to lookup what the error message might be

bwspenc commented 3 years ago

It's in MaterialBase::initStatefulProperties():

      mooseError(std::string("Material \"") + name() +
                 "\" provides one or more stateful "
                 "properties but initQpStatefulProperties() "
                 "was not overridden in the derived class.");
lindsayad commented 3 years ago

I don't think this should be totally removed. There was a reason @rwcarlsen added this message in the first place (see a9dc065a3e4 for the advent of the error). In a single subdomain simulation, what if a kernel requests an old property but the property declarer never initializes the old property? Wouldn't you want to be warned about that? I feel like in that instance you might run into uninitialized memory which would be terrible to debug for most users who wouldn't necessarily think to run valgrind when they start getting non-deterministic results from their simulation. I would be supportive of converting this to a warning and reference the possible valid use case of subdomain restriction.

rwcarlsen commented 3 years ago

We should definitely not remove the error. Converting to warning would be okay. But since this has been so sparsely reported and that error has been active for years, I'd wager that the error gives true-positives more often than not.

Most of the info to track these mappings should already exist. It is just a matter of plumbing it all together. We don't actually need to modify anything like MaterialPropertyStorage, etc. We just need to add the capture of active/relevant blocks in MaterialPropertyInterface when properties are retrieved - and then loop over all the objects that inherit from it to collect the block+prop combos that are needed and compare to block+prop combos provided by materials.

bwspenc commented 3 years ago

Giving a warning would at least allow us to run our model. I think that would be the best "quick fix" for this. While we're at it, it would be helpful to print out what the name of the problematic property is. I'll go ahead and make a PR that makes that change.

bwspenc commented 3 years ago

I guess the material name is printed out. It might be helpful to print out the actual properties too, although maybe that isn't that useful.