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

getADMaterialProperty does not report nonexistent material property #21867

Closed
joshuahansel opened this issue Aug 17, 2022 · 11 comments · Fixed by #22034
Closed

getADMaterialProperty does not report nonexistent material property #21867

joshuahansel opened this issue Aug 17, 2022 · 11 comments · Fixed by #22034
Assignees
Labels
P: normal A defect affecting operation with a low possibility of significantly affects. T: defect An anomaly, which is anything that deviates from expectations.

Comments

@joshuahansel
Copy link
Contributor

joshuahansel commented Aug 17, 2022

Bug Description

When using getADMaterialProperty and the user passing a nonexistent material property, it seems a zero value is used silently instead of reporting to the user that the property does not exist.

Steps to Reproduce

Run the following input file and observe that it runs despite the usage of nonexistent material properties:

(See first comment. I uploaded wrong file originally, and now Github is not letting me upload a different one to the description: "Something went really wrong, and we can't process that file.")

Note that the non-AD version getMaterialProperty does not have this issue: if in the input file above, you use the non-AD version of the kernels (remove "AD" from their names), you'll get an error message.

Impact

This is dangerous because there is no indication to the user of an issue.

@joshuahansel joshuahansel added T: defect An anomaly, which is anything that deviates from expectations. P: normal A defect affecting operation with a low possibility of significantly affects. labels Aug 17, 2022
@joshuahansel
Copy link
Contributor Author

zero_property.txt

@dschwen
Copy link
Member

dschwen commented Aug 17, 2022

Not by design. The code has explicit checks for non existing AD properties.

template <typename T>
ADMaterialProperty<T> &
MaterialData::getADProperty(const std::string & name)
{
  auto prop_id = getPropertyId(name);
  resizePropsAD<T>(prop_id);
  auto prop = dynamic_cast<ADMaterialProperty<T> *>(_props[prop_id]);
  if (!prop)
  {
    // We didn't find an AD material property so we're going to error out. But we can check to
    // see whether there is a regular property of the same name in the hope that we can give the
    // user a more meaningful error message
    auto regular_prop = dynamic_cast<MaterialProperty<T> *>(_props[prop_id]);
    if (regular_prop)
      mooseError("The requested AD material property " + name +
                 " is declared as a regular material property. Either retrieve it as a regular "
                 "material property with getMaterialProperty or declare it as an AD property wtih "
                 "declareADProperty");
    else
      mooseError("Material has no property named: " + name);
  }
  return *prop;
}

@joshuahansel
Copy link
Contributor Author

It looks like if there's no default value, non-AD properties end up doing this:

  const auto name = _get_suffix.empty()
                        ? static_cast<const std::string &>(name_in)
                        : MooseUtils::join(std::vector<std::string>({name_in, _get_suffix}), "_");

  checkExecutionStage();
  checkMaterialProperty(name);

  // mark property as requested
  markMatPropRequested(name);

  // Update the boolean flag.
  _get_material_property_called = true;

  // Does the material data used here matter?
  _material_property_dependencies.insert(material_data.getPropertyId(name));

  // Update consumed properties in MaterialPropertyDebugOutput
  addConsumedPropertyName(_mi_moose_object_name, name);

  return material_data.getProperty<T>(name);

whereas AD properties do this:

    // Does the material data used here matter?
    _material_property_dependencies.insert(material_data.getPropertyId(prop_name));

    return material_data.getADProperty<T>(prop_name);

@dschwen
Copy link
Member

dschwen commented Aug 17, 2022

Are you comparing getMaterialPropertyByName to getADMaterialProperty?

@joshuahansel
Copy link
Contributor Author

I'm comparing the else branch in getMaterialProperty (which calls getMaterialPropertyByName) to the else branch in getADMaterialProperty (which does not).

@dschwen
Copy link
Member

dschwen commented Aug 17, 2022

Can you just link to the the code? Are you in MaterialPropertyInterface?

@joshuahansel
Copy link
Contributor Author

I don't know how to link to a specific line, but yes, MaterialPropertyInterface: https://github.com/idaholab/moose/blob/next/framework/include/materials/MaterialPropertyInterface.h

@joshuahansel
Copy link
Contributor Author

joshuahansel commented Aug 17, 2022

Line 752 vs. 795

@lindsayad
Copy link
Member

can look into this when I'm back

@lindsayad lindsayad self-assigned this Aug 24, 2022
@joshuahansel
Copy link
Contributor Author

I got GOT again yesterday! @lindsayad I'll go ahead and have a go at this unless you object.

@lindsayad
Copy link
Member

Forgot about this. Yea go ahead if you want to! If you run into problems let me know

joshuahansel added a commit to joshuahansel/moose that referenced this issue Sep 7, 2022
- Added getADMaterialPropertyByName(name, mat_data)
- Added getGenericMaterialPropertyByName(name, mat_data)
- Made getMaterialPropertyByName(name) and
  getADMaterialPropertyByName(name) call
  getGenericMaterialPropertyByName(name) instead of the
  other way around

Refs idaholab#21867
joshuahansel added a commit to joshuahansel/moose that referenced this issue Sep 7, 2022
joshuahansel added a commit to joshuahansel/moose that referenced this issue Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P: normal A defect affecting operation with a low possibility of significantly affects. T: defect An anomaly, which is anything that deviates from expectations.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants