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

Addition: Cleanup logic. #75

Merged
merged 22 commits into from
Nov 22, 2024
Merged

Conversation

jmark
Copy link
Contributor

@jmark jmark commented Nov 21, 2024

This PR adds clean up logic which frees wrapped forest objects before the program shuts down.

Copy link
Collaborator

@JoshuaLampert JoshuaLampert left a comment

Choose a reason for hiding this comment

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

I can't really judge the t8code, MPI and GC stuff and I don't have the time to try to understand everything in detail, but I left some general comments below. What I was wondering from an abstract point of view is why does T8CODE_OBJECT_TRACKER need to be a Dict having the unique_id of the wrapper and the wrapper itself? Will not T8CODE_OBJECT_TRACKER[unique_id].unique_id == unique_id? I mean, isn't this redundant information? Couldn't we also just use a Vector{ForestWrapper} for T8CODE_OBJECT_TRACKER?

Project.toml Outdated Show resolved Hide resolved
src/T8code.jl Outdated Show resolved Hide resolved
src/T8code.jl Outdated Show resolved Hide resolved
src/T8code.jl Outdated Show resolved Hide resolved
src/T8code.jl Outdated Show resolved Hide resolved
src/T8code.jl Outdated Show resolved Hide resolved
src/T8code.jl Outdated Show resolved Hide resolved
@jmark
Copy link
Contributor Author

jmark commented Nov 22, 2024

I can't really judge the t8code, MPI and GC stuff and I don't have the time to try to understand everything in detail, but I left some general comments below. What I was wondering from an abstract point of view is why does T8CODE_OBJECT_TRACKER need to be a Dict having the unique_id of the wrapper and the wrapper itself? Will not T8CODE_OBJECT_TRACKER[unique_id].unique_id == unique_id? I mean, isn't this redundant information? Couldn't we also just use a Vector{ForestWrapper} for T8CODE_OBJECT_TRACKER?

First, thank you very much for taking your time and doing a review!

I need the mapping from a unique id to a ForestWrapper for the MPI parallel case. The unique id from the root rank is broadcasted to the other ranks. This way, it is assured that all ranks finalize the same forest.

Indeed, the unique id does not have to be stored in the ForestWrapper. I removed this field.

@JoshuaLampert
Copy link
Collaborator

Indeed, the unique id does not have to be stored in the ForestWrapper. I removed this field.

Yes, removing the field from the ForestWrapper sounds reasonable to me. Thanks!

@JoshuaLampert
Copy link
Collaborator

You will also need to bump the MPI version to v0.20.6 in test/Project.toml.

Project.toml Outdated Show resolved Hide resolved
test/Project.toml Outdated Show resolved Hide resolved
jmark and others added 2 commits November 22, 2024 15:36
Co-authored-by: Joshua Lampert <[email protected]>
Co-authored-by: Joshua Lampert <[email protected]>
@jmark
Copy link
Contributor Author

jmark commented Nov 22, 2024

@JoshuaLampert Anything else to do?

@JoshuaLampert
Copy link
Collaborator

@JoshuaLampert Anything else to do?

There is an unanswered question above.

@jmark
Copy link
Contributor Author

jmark commented Nov 22, 2024

@JoshuaLampert Anything else to do?

There is an unanswered question above.

Strange. My GH UI tells me all green. What else is there to address?

@JoshuaLampert
Copy link
Collaborator

Strange. My GH UI tells me all green. What else is there to address?

GH didn't show me your message above before. Whatever, this is good to go from my side.

@jmark jmark merged commit 235b47f into main Nov 22, 2024
15 checks passed
@jmark
Copy link
Contributor Author

jmark commented Nov 22, 2024

@JoshuaLampert Always a treat working with you! Thanks! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants