-
Notifications
You must be signed in to change notification settings - Fork 85
adds support for generic sets, elem sets to exodus3.in.py #743
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
base: master
Are you sure you want to change the base?
Conversation
|
@gsjaardema hey Greg -- is this the right place for this? Not sure if anyone is watching here. |
|
@ecoon Yes, this is the right place. I was on vacation the day you posted and just got back. Will look at it hopefully today. |
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.
Looks good. Thanks for doing this. There are a couple comments.
Not sure of best path forward for the EX_SIDE_SET . Maybe for now just error out in that case and we fix it later.
Only mandatory change would be the addition of None
| set_inds = (ctypes.c_longlong * num_indices)() | ||
| else: | ||
| set_inds = (ctypes.c_int * num_indices)() | ||
| EXODUS_LIB.ex_get_set(self.fileId, obj_type, set_id, ctypes.byref(set_inds)) |
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 think this needs an extra parameter None at the end to take care of the set_extra_list. For a sideset, this holds the local_face part of the side definition.
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.
Yes, otherwise we get a warning (but it works) -- I fixed this in the Amanzi patch but I guess not here. Will fix.
|
|
||
| # -------------------------------------------------------------------- | ||
|
|
||
| def get_set_indices(self, object_type, object_id): |
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 don't think this will work for EX_SIDE_SET since the indices for that are a pair of element and local_face_number. Similarly for theput_set_indices
We can maybe add a test for object_type == EX_SIDE_SET and
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.
That's right. This function should either error or have a more generic API (equivalent to get_side_set()'s current implementation) in the case of EX_SIDE_SET.
|
In general @gsjaardema do you have a preference of:
Either way I'll fix the new generic API to work for EX_SIDE_SET as well. |
We need to leave the old API in case people are using it. I don't want to break existing uses... |
Note that your contribution guidelines suggest this will get pre-populated with info, but that is not working correctly.
This is a small addition to the exodus3.in.py wrapper to support generic addition of sets to exo files via python. This calls the "generic" exodus API of:
whereas the current implementation only uses the "node" or "sideset" specific API (e.g. ex_put_node_set()) which is deprecated according to the Exodus source. It would be trivial to remove or update the node- or side-set based versions to use only non-deprecated code, but that is not done here -- this only adds new functions. I'm happy to do that as well if you would like.
Note that corresponding get_* methods are also added.
This also only touches the python3 wrappers, not the python2 wrappers. Are you still supporting python2?