-
Notifications
You must be signed in to change notification settings - Fork 22
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
Allow nogil for most types of methods #181
base: master
Are you sure you want to change the base?
Conversation
This requires a change in conversion providers. +Some other cleanups.
I will fix tests because of newer typing syntax |
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 looks good to me. Any idea what sort of performance improvements we are likely to see?
Probably none out of the box. This brings some responsibilities for the user with it but you can decide which functions are safe to use. |
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.
Not super confident that I understand all parts. Would be great if in general methods would contain a brief comment e.g.the new _split one etc.
This is a breaking change. Be prepared to fix OpenMS if you merge this. |
Given that we already need to do some fixes for OpenMS to get it to work with the current develop version of autowrap, I'm good merging this in and planning to have the core group sit down and fix at some point in the future. |
what needs to be done to work with the current develop version of autowrap otherwise? |
and what would need to be fixed? |
|
This requires a change in conversion providers.
+Some other cleanups.