-
Notifications
You must be signed in to change notification settings - Fork 91
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
Refactor r-tests for C-bindings interfaces #148
Refactor r-tests for C-bindings interfaces #148
Conversation
…the python driver code
… the python driver code
… + add docstrings
…m driver_utilities.py
…odyn_inflow_library.py library
"moordyn": "libmoordyn_c_binding" | ||
} | ||
basename = module_map.get(module_name.lower(), f"lib{module_name}_c_binding") | ||
build_path = Path("..").joinpath(*[".."] * 4, "build") |
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 would prefer we didn't hard code this to the build
directory. But I think that is what we already had.
print(f"Cannot load MoorDyn test file: {e}") | ||
sys.exit(1) | ||
|
||
def _initialize_library(self) -> moordyn_library.MoorDynLib: |
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.
After using this new change, I have some feedback. Consider it a suggestion or food for thought more than anything.
The Python interfaces to OpenFAST are in an interesting scenario in that they are a way to access a library but they aren't the library itself. From a design perspective, we would generally want to make it opaque to the user (Python programmer) that there's a Fortran module running behind the scenes. However, I think there is some benefit to maintaining a level of consistency with OpenFAST so that we can debug and move across entry points (Python vs C++ vs Fortran) easier. In that regard, I suggest to separate the concerns of this function. Instead of initializing the Python interface and attaching the compiled shared library to the CDLL object + reading the MoorDyn inputs and running MoorDyn.init, I think it would be better scoped to instantiate the interface and attach to the compiled library opaquely and then require that the user (Python programmer making an outer level interface) call MoorDyn init explicitly. As OpenFAST developers, we are accustomed to calling the module functions (i.e. MoorDyn init) but we aren't as accustomed to dealing with the ctypes / CDLL stuff.
I'm happy to have a more in depth chat about it or open a discussion. I think it's important to get right so that we can establish a good pattern and be consistent throughout the other modules.
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.
Good thoughts on this. I'm open to revising how this is done.
@faisal-bhuiyan can you confirm that the error handling is working here? I've changed these two lines in ErrStat = ErrID_None
ErrMsg = "" to this: ErrStat = ErrID_Fatal
ErrMsg = "Big ole error" And I expected it to error in the |
That should trigger an error. I'm a little surprised it doesn't -- maybe we don't capture it correctly on the |
Major refactor of the driver codes for Python-based libraries to the C-bindings interfaces. These driver codes are essentially regression tests for several modules of OpenFAST. Following tests were refactored in this work