STK: wrong inline keyword in front of BulkData::relation_exist

[bartgol]

Description

The declaration is at this line, but the function is defined here. Since the fcn is marked inline, it never makes it in the compiled object.

Notice that the bug is not in master (the inline function definition is in the hpp file), but it is present in develop!

Steps to Reproduce

• SHA1: 71f1963
• Configure script: enable Trilinos_ENABLE_STKMesh
• build
• go in packages/stk/stk_mesh/stk_mesh/base

$ nm -C CMakeFiles/stk_mesh_base.dir/BulkData.cpp.o | grep relation_exist
$ 

• go in BulkData.hpp, remove inline keyword, rebuild:

$ make BulkData.cpp.o
[ 93%] Building CXX object packages/stk/stk_mesh/stk_mesh/base/CMakeFiles/stk_mesh_base.dir/BulkData.cpp.o
$ nm -C CMakeFiles/stk_mesh_base.dir/BulkData.cpp.o | grep relation_exist
0000000000009cb0 T stk::mesh::BulkData::relation_exist(stk::mesh::Entity, stk::topology::rank_t, unsigned int)
(bartgol) lbertag@s1007989:[base]$ 

We should either move the impl in the hpp file (as it's done in master), or remove the inline keyword.

[alanw0]

Thanks for the bug report.
That function (and several others) were moved from the header to the .cpp file recently. Obviously we missed that inline keyword, we will remove the inline keyword asap.
(It's weird that it hasn't shown up in any of our local nightly builds, in the stk home repo...)

[bartgol]

I don't see that function used at all in the stk folder. Maybe that's why it wasn't caught.

[alanw0]

I don't see that function used at all in the stk folder. Maybe that's why it wasn't caught.

I thought it was used by the 'legacy' sierra framework, but it looks like it isn't used there either. I already started a commit to remove the inline keyword, but I think I'll just deprecate the method instead.

[bartgol]

I don't see that function used at all in the stk folder. Maybe that's why it wasn't caught.

I thought it was used by the 'legacy' sierra framework, but it looks like it isn't used there either. I already started a commit to remove the inline keyword, but I think I'll just deprecate the method instead.

We're currently using this method in Albany (hence, the link error we got that made me spot it). If you want to deprecated it and then remove it, we will have to find a new way to achieve the result we need. Do you have any suggestion? Currently, it's used like this:

   // 'parent' and 'child' are stk::mesh::Entity, and rank is the rank of the child entity.
   // We're trying to see if the entity 'child' is a sub-entity of 'parent'
    const auto num_children = bulkData->num_connectivity(parent,rank);
    const auto children = bulkData->begin(parent,rank);
    for (unsigned isub=0; isub<num_children; ++isub) {
      if (bulkData->relation_exist(parent,rank,isub) && children[isub]==child)
        return isub;
    }

Maybe there's already another functionality to achieve this?

[alanw0]

ok, interesting. That code is actually not quite correct.
The 3rd argument to 'relation_exist' should not be isub.
Correct code would be this:

    const auto num_children = bulkData->num_connectivity(parent,rank);
    const auto children = bulkData->begin(parent,rank);
    const auto ordinals = bulkData->begin_ordinals(parent,rank);
    for (unsigned isub=0; isub<num_children; ++isub) {
      if (bulkData->relation_exist(parent,rank,ordinals[isub]) && children[isub]==child)
        return isub;
    }

Think of the example where parent is a hex8 element and rank is FACE_RANK. The ordinal (referred to as relationId in the relation_exist argument) is the face local-index relative to the hex topology, and values can be in the range [0..5]. But often a hex has only a couple of its face-entities (e.g. often only external-boundary faces exist) so the ordinal is unlikely to be the same as the loop-index (isub in your loop), unless possibly if you have created all faces (including internal) in your mesh.
In the above code there is a redundancy because relation_exist is internally doing the loop over num_children. So your code would be equally correct and more efficient if it was this:

    const auto num_children = bulkData->num_connectivity(parent,rank);
    const auto children = bulkData->begin(parent,rank);
    for (unsigned isub=0; isub<num_children; ++isub) {
      if (children[isub]==child)
        return isub;
    }

Let me know if I'm mis-interpreting your query. It seems as though you just want to know if the child is connected to the parent, and you aren't actually concerned with what the ordinal/relationId is.

[bartgol]

No, you interpreted it correctly. This part of STK was always not super clear to me, namely the storage of entity relations (which could be, as you said, less than the upper limit dictated by topology).

Ok, since the check on relation_exist is pointless, I will remove it and use your version. This way we won't be in the way of your deprecation/removal of the relation_exist method.

[alanw0]

ok great.
Maybe I shouldn't be so hasty in deprecating. We can easily add a free-function that does that relation_exist check if we find anyone else who needs it.
In general we are eager to deprecate BulkData methods because BulkData is an overly large monster of a class.

[bartgol]

Yes, I agree. I already changed out code to use the impl you suggested. No need to keep the method.

I'll leave the issue open, and I will let you decide whether to fix it or not (and close the issue or not).

[alanw0]

ok sounds good. I will get a change in to deprecate that method. (And provide a free-function replacement later if any other users of it appear.)
It will be early next week.

[alanw0]

@bartgol I believe this is now resolved, the linked pull-request has deprecated this method. Thanks!