-
Notifications
You must be signed in to change notification settings - Fork 25
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
MPhys multibody #192
MPhys multibody #192
Conversation
@bburke38 Regarding the format failure... I did python -m black mphys_meld.py and it reformatted it. Is that all that would be needed to pass this test? In any case, there was a tweak on the MPhys side where builder.py's get_tagged_indices will now raise NotImplementedError if it hasn't been implemented in an aero/struct. I added try/catch loops to the MELD builder. If it sounds okay with you, I'll push the new version with that change, and with the reformatting. |
@Asthelen Yes that should take care of the formatting issues. Feel free to push that commit. |
…s now that default now raises NotImplementedError
Sounds good--just pushed that commit. For the record, an example using multiple bodies is provided here: https://github.com/Asthelen/mphys/tree/tags_for_multi_body/examples/aerostructural/crm_with_tail/vlm_meld_pyshell That MPhys PR has been approved, but hasn't fully gone through yet, so for now it just exists on my fork. |
@bburke38, can we add some of the mphys users as reviewers to this PR before merging, as this likely affects our work (@A-CGray, @kejacobson etc.)? I just want to make sure this PR works for us on the TACS side |
funtofem/mphys/mphys_meld.py
Outdated
aero_gid = self.aero_builder.get_tagged_indices( | ||
self.body_tags[i]["aero"] | ||
) | ||
except: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make these excepts
more specific for the expected error? Basically except NotImplementedError
funtofem/mphys/mphys_meld.py
Outdated
struct_gid = self.struct_builder.get_tagged_indices( | ||
self.body_tags[i]["struct"] | ||
) | ||
except: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
funtofem/mphys/mphys_meld.py
Outdated
self.struct_dof_indices = [] # for dof | ||
for i in range(3): # accounts for xyz of each grid coordinate | ||
self.struct_coord_indices = np.hstack( | ||
[self.struct_coord_indices, struct_gid * 3 + i] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is breaking for me when struct_gid
is a list
rather than an array
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should probably make sure this is cast to an array first with np.atleast1d
in case the user passes back the indices in a list from get_tagged_indices
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference here's the error:
if aero_gid is not None: # use grid ids from aero_builder's get_tagged_indices
self.aero_coord_indices = []
for i in range(3):
self.aero_coord_indices = np.hstack(
> [self.aero_coord_indices, aero_gid * 3 + i]
)
E TypeError: can only concatenate list (not "int") to list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't push to your fork @Asthelen but I think this code would fix the issue:
# account for xyz of each node (create array of [x_0, ..., x_N, y_0, ..., y_N, z_0, ..., z_N])
struct_gid = np.atleast_1d(struct_gid)
num_struct_gid = len(struct_gid)
self.struct_coord_indices = np.tile(struct_gid, 3)
for ii in range(3):
self.struct_coord_indices[
ii * num_struct_gid : (ii + 1) * num_struct_gid
] += ii
# account for each structural DOF of each node (create array of [u_0, ..., u_N, v_0, ..., v_N, w_0, ..., w_N, ...])
self.struct_dof_indices = np.zeros(num_struct_gid * struct_ndof)
for ii in range(struct_ndof):
self.struct_dof_indices[
ii * num_struct_gid : (ii + 1) * num_struct_gid
] = (struct_gid * struct_ndof + ii)
self.struct_coord_indices = list(
np.sort(self.struct_coord_indices).astype("int")
)
self.struct_dof_indices = list(
np.sort(self.struct_dof_indices).astype("int")
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to tweak the first part to make it consistent with what was there... thoughts on this instead?
# account for xyz of each node (i.e., [x_0, y_0, z_0, ..., x_N, y_N, z_N])
struct_coord_indices = np.zeros(struct_nnodes * 3, dtype=int)
for ii in range(3):
struct_coord_indices[
ii * struct_nnodes : (ii + 1) * struct_nnodes
] = struct_gid * 3 + ii
# account for each structural DOF of each node (i.e., [u_0, v_0, w_0, ..., u_N, v_N, w_N])
struct_dof_indices = np.zeros(struct_nnodes * struct_ndof, dtype=int)
for ii in range(struct_ndof):
struct_dof_indices[
ii * struct_nnodes : (ii + 1) * struct_nnodes
] = (struct_gid * struct_ndof + ii)
struct_coord_indices = list(
np.sort(struct_coord_indices)
)
struct_dof_indices = list(
np.sort(struct_dof_indices)
)
Then something similar for aero indices too. I also have the np.atleast_1d around the get_tagged_indices calls now. The np.atleast_1d fixed the list vs. array issue, so this gives the same answer as before but might be better than using hstack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this sort a bit later turns it into [0, 1, 2, 6, 7, 8] though:
self.struct_coord_indices = list(
np.sort(self.struct_coord_indices).astype("int")
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ohhhhhh right, sorry I totally missed that. In that case I might actually remove the comments I added since the ordering in the comment doesn't match the actual ordering at the point in the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the other hand, you could also just create the array in that sorted order directly with something like this:
struct_gid=np.sort(np.at_least1d(struct_gid))
# account for xyz of each node (i.e., [x_0, y_0, z_0, ..., x_N, y_N, z_N])
struct_coord_indices = np.zeros(struct_nnodes * 3, dtype=int)
for ii in range(3):
struct_coord_indices[ii::3] = struct_gid + ii
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, @A-CGray's method seems like a more straightforward alternative here. Plus it gets rid of the unnecessary sort step at the end. You could actually even get rid of the at_least1d method now, since the np.sort in the beginning should handle the case of a list
I think that the last line should actually be struct_coord_indices[ii::3] = 3 * struct_gid + ii
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I agree too... I figured it could've been cleaned up, but didn't pull the trigger on that. Latest push has that change, and I also changed *_gid to *_node_ids.
funtofem/mphys/mphys_meld.py
Outdated
self.aero_coord_indices = [] | ||
for i in range(3): | ||
self.aero_coord_indices = np.hstack( | ||
[self.aero_coord_indices, aero_gid * 3 + i] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above
funtofem/mphys/mphys_meld.py
Outdated
aero_gid=None, # list of aero grid IDs to couple | ||
aero_nnodes=None, | ||
struct_gid=None, # list of struct grid IDs to couple |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be more correct to change gid
to tags
throughout the code to more accurately reflect the changes made to OpenMDAO/mphys#158 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot to respond to this... to me, tags
sounds like a group of nodes corresponding to a boundary, property group, etc. I'm open to name changes, but would prefer something relating to node
ids rather than tag
ids?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, this again is me misunderstanding the code, I thought this input was the tags list, but now I see that the MeldBuilder
deals with those and these are the actual node indices. node_ids
would be slightly clearer.
I pushed a new version with the docstring, and atleast_1d around the get_tagged_indices calls. The struct_coord_indices, etc. are still the same except for the longer comments (pointing to indices of x0, y0, z0, etc.). Maybe it's still worth cleaning up/avoiding hstacks (something like @A-CGray's suggestion) but I think it should work as is. I'm happy to keep tweaking things. Or if it helps, I might be able to add one/some of you as collaborators if you'd like to push to my fork. At the moment, I'm not sure how easily you all can test full aerostructural cases though (since the example uses pyshell). |
@Asthelen if you re-merge your branch with master again the CI should probably pass this time. We recently had an issue with a new Cython release that was breaking the instal process |
Good to know--thanks. I wondered if the issue was on my end or not. Fork is now synced, so hopefully it passes this time. |
@bburke38 Can we get a version bump whenever this is merged in? |
@timryanb Will do, I'm going to go ahead and merge it now. Sorry for the delay. |
mphys_meld.py has been modified for optional coupling of multiple bodies. When body_tags is specified, the builder now uses an MPhys builder function "get_tagged_indices" to couple specific sets of aero and structural nodes with separate MELD instances. Without body_tags specified, it by default couples all nodes like before.
Addresses MPhys issue 116 which seeks to couple multiple bodies in MPhys.