Skip to content
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

Fix finalise in boutpp #3053

Merged
merged 6 commits into from
Jan 9, 2025
Merged

Fix finalise in boutpp #3053

merged 6 commits into from
Jan 9, 2025

Conversation

dschwoerer
Copy link
Contributor

Resolves #3041

This is to ensure that all MPI routines are called before MPI_Finalise
is called.

Resolves: gh#3041
The list from gc was incomplete.
Using a custom weakref list ensures we can free all objects before
MPI_Finalize is called.

This reverts 0e4314c, which was not working as it relied on the
same, incomplete, list of objects.

Resolves: gh#3041
Comment on lines +58 to +63
def __cinit__(self, {{ init_args}}):
global _allobjects
_allobjects.append(weakref.ref(self))
{% if not uniquePtr and cobj %}
self.isSelfOwned = {{ defaultSO }}
self.cobj = NULL
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option that might be easier to read would be to just template this function

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you mean move this to a separate file?

I do not think it would get much easier.
Certainly the file is a bit long and hard to read, but I am not sure moving this macro to a different file helps a lot with cleaning up.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I meant just have something like:

{% macro class_cinit(init_args="*args, **kwargs", defaultSO=True) %}
cdef c.bool isSelfOwned
cdef object __weakref__

def __cinit__(self, {{ init_args }}):
  ...

{% endmacro %}

and use like:

cdef class Mesh:
   ...

{{ class_cinit | indent }}

Avoids inheritance, reduces the duplication from the __cinit__ method, and keeps the class definitions looking normal

@bendudson bendudson merged commit 2ad21ae into next Jan 9, 2025
24 of 26 checks passed
@bendudson bendudson deleted the fix-finalise branch January 9, 2025 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants