Tags for multi body

[Asthelen]

Added get_tagged_indices to builder.py for cases with multiple coupled bodies. Addresses issue #116

[timryanb]

@Asthelen, could we add a note to the docstring describing what returning None by default implies?

Does this mean that all nodes will be used or none of them?

If its the latter does this represent a breaking change for older builders?

[timryanb]

Are there any examples using this feature? I thought I remembered seeing one in the slides @Asthelen presented a couple of weeks ago

[timryanb]

Where is this method called? In the meld builder or the scenario level?

[Asthelen]

I currently have my meld builder calling this function (as well as a geometry builder or two, and the "controls" builder I've been trying out). If None is returned, then the builder assumes it hasn't been implemented and will just use all nodes. So, perhaps the note would say "Returning None indicates that this function hasn't been implemented in the builder" or something along those lines?

As for an example: the one I have set up uses VLM and pyShell, which of course aren't open source so I don't have it on my MPhys fork. I'm happy to share it though.

[kejacobson]

Following Andrew's comment, we can have it be backwards compatible. None = not implemented. Return a zero length list if implemented but the builder doesn't have any nodes on this rank.

[timryanb]

@kejacobson, that makes sense to me. As long as we make it clear in the docstrings None -> "all nodes". Since the current syntax seems ambiguous

[timryanb]

@Asthelen, if you're comfortable sharing it. Could we add that example to this PR? It'll make understanding how to use the feature easier (even if we can't run it).

Could you also make a PR with your changes to smdogroup/funtofem on the meld side?

[timryanb]

In fact, maybe we can avoid ambiguity all together and implement the base method as below:

    def get_tagged_indices(self, tags):
        """
        Method that returns grid IDs for a list of body/boundary tags.
        Parameters
        ----------
        tags : list of integers or strings
        Returns
        -------
        grid_ids : list
            list of grid IDs that correspond to given body/boundary tags
        """
        nnodes = self.get_number_of_nodes()
        all_node_ids = np.arange(nnodes)
        return list(all_node_ids)

[Asthelen]

That's something I considered (and may have tried briefly). Would have required a new numpy import, and anything that uses this function wouldn't be aware that it hasn't been implemented, but maybe none of that is an issue.

[Asthelen]

Regarding the FUNtoFEM PR: I'll plan on doing that soon. I think it needs tweaked since the version I started with didn't have the linearized option (which should just be a few lines). And if this get_tagged_indices function is changed in the above way then the MELD builder could be simplified a bit.

As for the example: I think I should be able to add that to the PR.

[timryanb]

That's something I considered (and may have tried briefly). Would have required a new numpy import, and anything that uses this function wouldn't be aware that it hasn't been implemented, but maybe none of that is an issue.

I agree with your concern that this masks if the method hasn't been implemented. It may not be the users intent to use all of the nodes. Can we make this method return a NotImplementedError instead? That seems more explicit than None

To preserve backwards compatibility, we could have the get_tagged_indices method only called if the user actually specifies tags at the top level for MELD. Otherwise, it skips the method and just uses all nodes. That way an error only occurs if the user is trying to pass tags w/o that feature existing in the builder

[Asthelen]

I think that would be problematic if get_tagged_indices has only been implemented in one of the aero/struct builders. For instance, currently, if I was to couple a VLM wing (body ID 0) to TACS and wanted the tail (body ID 1) to remain rigid, I think these body tags would work:

[{'aero': [0], 'struct': None}]

There, the TACS builder's get_tagged_indices would return None (or a list of all grid IDs), but it'd error out if the default returned NotImplementedError.

[timryanb]

I think that would be problematic if get_tagged_indices has only been implemented in one of the aero/struct builders. For instance, currently, if I was to couple a VLM wing (body ID 0) to TACS and wanted the tail (body ID 1) to remain rigid, I think these body tags would work:

[{'aero': [0], 'struct': None}]

There, the TACS builder's get_tagged_indices would return None (or a list of all grid IDs), but it'd error out if the default returned NotImplementedError.

An error here still makes sense to me. You're still passing a tag to TACS, even if its a "NULL" tag. So the user should get an error.

I also don't like the use of None in the tag for this example as shorthand for "all", it's really confusing. Consider using something like -1 or something else

[Asthelen]

I guess that's fair. It would be easy to make your builder just return a list of all nodes, if you wanted to, until you had that implemented.

And yeah that None in the body tags list wouldn't be used anywhere, so I could've made that anything. In the MELD builder, there are aero_gid and struct_gid variables that are None when not using body tags... which I think is a little different and maybe fine. I can point them out once I have the builder pushed to my fork.

[Asthelen]

Here's a link to that current MELD builder, if you want to take a look: https://github.com/Asthelen/funtofem/blob/mphys_multibody/funtofem/mphys/mphys_meld.py

In my previous comment, I was referring to how the MeldBodyInstance class is used. When a list of bodies isn't given, I just have it using the numbers of nodes from the two builders, and aero_gid and struct_gid end up being None https://github.com/Asthelen/funtofem/blob/5ff3d3649eaa82b6e6e506c3b7b755cbcae83eb7/funtofem/mphys/mphys_meld.py#L462

[timryanb]

I don't have problem with the use None in this case, since it's used as a default input argument and never passed explicitly as an input.

[Asthelen]

@timryanb The multi-body example has been added to the PR.

As for the get_tagged_indices: @kejacobson mentioned he's more in favor of it returning None. Do you suppose it's worth having the MELD builder just print a warning if a builder's get_tagged_indices returns None, so that it's more of a "soft" error while still keeping the user informed? I put in a PR for the MELD builder, but could plan on adding that there.

[timryanb]

I'm not sure what the benefit of returning None is here? I'd really recommend making this an error. If you want this handled as a soft error in MELD, specifically, the proper way would be with a try/except statement that catches a NotImplementedError

[timryanb]

@Asthelen @kejacobson I can let it go if you guys feel that this is needed. Just trying to understand the intent here

[kejacobson]

I'm not strongly opinionated on it, so I'm fine with making try-except blocks with raising exceptions too.

There are two other functions, the getters for the number of nodes and degrees of freedom that could also raise exceptions instead of return -1 in a similar way.

[timryanb]

I'm not strongly opinionated on it, so I'm fine with making try-except blocks with raising exceptions too.

There are two other functions, the getters for the number of nodes and degrees of freedom that could also raise exceptions instead of return -1 in a similar way.

I guess that depends on whether we want to require all builders to have a mesh associated with them.
I think two options for those methods would be: return 0 (implies if the method isn't implemented in the derived class the builder has no nodes) or raise an error (implies that the derived class must always explicitly define the number of nodes associated w/ it).

[kejacobson]

return 0 may not imply "not implemented" for the number of nodes because some cases like CFD surface meshes may not have nodes on every proc since the decomposition is based on the volume mesh, so some procs will return 0 as a valid response. This is why it is return -1 currently in the base class because that will never be valid. But I'm fine with making these other two functions also raise not implemented. If you try to get_number_of_nodes() on say a pycycle builder, you're trying to do something wrong and in situation where we want to stop the code. @Asthelen is going to push this change.