aspect icon indicating copy to clipboard operation
aspect copied to clipboard

fix invalid access to in.strain_rate

Open tjhei opened this issue 2 years ago • 7 comments

We set the size of MaterialModelInputs::strain_rate to zero when it is not supplied, but it turns out that we ignore this fact and still access these entries. It seems that the compiler does not reliably detect this, #see 4862.

I see the following options:

  1. Always set to the correct size but fill with NaN. Problem: we check in.strain_rate.size() in a few places.
  2. Provide an access function with an Assert inside and make strain_rate private.
  3. Always fill the strain rate, especially considering that we have more situations where we do need the strain rate than we used to (it used to be only for viscosity)

tjhei avatar Jun 26 '22 20:06 tjhei

@naliboff Does the original idea "only supply strain_rate if we need the viscosity" make any sense still?

@anne-glerum I am also curious what your thoughts about this are.

tjhei avatar Jun 26 '22 20:06 tjhei

Hi @tjhei I think we could remove setting the size of the strain rate to 0 and make sure to request the minimum amount of material properties from the material model with in.requested_properties as used in #4248. Setting the strain rate to size 0 was a flag for the material models to not compute the viscosity, and that task can be/is taken over by the requested properties.

anne-glerum avatar Jun 27 '22 08:06 anne-glerum

Ok, but how do we detect if we need to fill strain_rate with real information? We need to ask the material model if any of the requested properties requires the strain rate to compute...

tjhei avatar Jun 27 '22 18:06 tjhei

So there is ModelDependence but it seems to be incomplete (no reaction rates, etc.) and unused so far. But that framework could be used to compute what inputs we need to fill based on the requested properties.

tjhei avatar Jun 27 '22 19:06 tjhei

Ah I was thinking we just always fill the strain rate, but then in the material model decide to use it or not depending on the requested properties. Would it save a lot of time not to fill the strain rate?

anne-glerum avatar Jun 28 '22 07:06 anne-glerum

I don't know how big the difference will be, but the motivation behind bool compute_strain_rate was the desire to save computational time.

My main problem is that we need a minimal change for the 2.4 release now, without changing too much in the material model.

tjhei avatar Jun 28 '22 14:06 tjhei

Ah I thought the time saving was avoiding computing the viscosity.

What if, for now, when we fill the MaterialModelInputs (line 271 and 356 of material_model/interface.cc), we don't just check

 if (compute_strain_rate == false)
        this->strain_rate.resize(0);

but also check for reaction_outputs?

 if (compute_strain_rate == false && in.requested_properties != reaction_outputs)
        this->strain_rate.resize(0);

We should check, but I don't think the other properties (apart from viscosity) need the strain rate.

anne-glerum avatar Jun 30 '22 07:06 anne-glerum

@gassmoeller With your recent change to always compute the strain rate (#5268), can this be closed?

bangerth avatar Jul 14 '23 05:07 bangerth

Yes, I think this was fixed by #5268.

gassmoeller avatar Jul 14 '23 05:07 gassmoeller