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: generalize hard-coded Cint to MPI_Comm in t8_forest_get_mpicomm. #78

Merged
merged 2 commits into from
Dec 5, 2024

Conversation

jmark
Copy link
Contributor

@jmark jmark commented Dec 5, 2024

This patch generalizes the return value of t8_forest_get_mpicomm. This is crucial when using OpenMPI as system library. The MPI Communicator handle is not an integer but a pointer to a struct.

Along the way, I also added other minor improvements.

@jmark jmark added the fix Fixes a bug. label Dec 5, 2024
@jmark jmark requested a review from JoshuaLampert December 5, 2024 15:16
@JoshuaLampert
Copy link
Collaborator

Looks good. Does this mean that executing julia --project generator.jl && ./fixes.sh in dev/ produces the Libt8.jl file?

@JoshuaLampert
Copy link
Collaborator

This reminds me of the following error: https://github.com/trixi-framework/Trixi.jl/actions/runs/12182527496/job/33981760954?pr=1844#step:7:11266. Could this be similar? It fails here:

T8code.jl/src/Libt8.jl

Lines 17842 to 17848 in 73877de

```c
void t8_geom_load_tree_data_vertices (t8_cmesh_t cmesh, t8_gloidx_t gtreeid, const void **user_data);
```
"""
function t8_geom_load_tree_data_vertices(cmesh, gtreeid, user_data)
@ccall libt8.t8_geom_load_tree_data_vertices(cmesh::Cint, gtreeid::Cint, user_data::Ptr{Ptr{Cvoid}})::Cvoid
end

To me the docstring looks like cmesh should be of type t8_cmesh_t and not of type CInt = UInt32 as indicated in the function call. Could this lead to the InexactError?

@jmark
Copy link
Contributor Author

jmark commented Dec 5, 2024

Looks good. Does this mean that executing julia --project generator.jl && ./fixes.sh in dev/ produces the Libt8.jl file?

Yes, indeed!

@jmark
Copy link
Contributor Author

jmark commented Dec 5, 2024

This reminds me of the following error: https://github.com/trixi-framework/Trixi.jl/actions/runs/12182527496/job/33981760954?pr=1844#step:7:11266. Could this be similar? It fails here:

T8code.jl/src/Libt8.jl

Lines 17842 to 17848 in 73877de

```c
void t8_geom_load_tree_data_vertices (t8_cmesh_t cmesh, t8_gloidx_t gtreeid, const void **user_data);
```
"""
function t8_geom_load_tree_data_vertices(cmesh, gtreeid, user_data)
@ccall libt8.t8_geom_load_tree_data_vertices(cmesh::Cint, gtreeid::Cint, user_data::Ptr{Ptr{Cvoid}})::Cvoid
end

To me the docstring looks like cmesh should be of type t8_cmesh_t and not of type CInt = UInt32 as indicated in the function call. Could this lead to the InexactError?

I actually fixed this issue a while ago on t8code side. For whatever reason this fix gone. I adapted the fixes.sh accordingly.

Thanks for pointing that out!

@JoshuaLampert
Copy link
Collaborator

Ok, thanks! Is this fix actually gone in t8code or only in T8code.jl? If the latter (and your fix in t8code is included in the t8code version we currently wrap), we could (should?) investigate why it's not in T8code.jl. (If the former, you could maybe investigate why it's gone in t8code as it would be nice to not need a manual fix on the T8code.jl end.)

@jmark
Copy link
Contributor Author

jmark commented Dec 5, 2024

Ok, thanks! Is this fix actually gone in t8code or only in T8code.jl? If the latter (and your fix in t8code is included in the t8code version we currently wrap), we could (should?) investigate why it's not in T8code.jl. (If the former, you could maybe investigate why it's gone in t8code as it would be nice to not need a manual fix on the T8code.jl end.)

Actually, my old fix was added to another file in t8code. The issue here was introduced in a new file. I pushed a fix upstream:
DLR-AMR/t8code#1319

But I recommend to merge this PR since the upstream fix won't be available soon and the fix here is idempotent to the fix upstream.

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.

Thanks!

@jmark jmark merged commit 3f7b1fc into main Dec 5, 2024
15 checks passed
@jmark jmark deleted the fix-mpicomm-return-value branch December 5, 2024 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants