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

Add functions for Cartesian process topology #227

Merged
merged 5 commits into from Jan 19, 2019

Conversation

Projects
None yet
3 participants
@samo-lin
Copy link

samo-lin commented Jan 14, 2019

Addition of the following functions that are fundamental to create and use a Cartesian process topology:

Closes #225

for i in 0:2
neighbors = MPI.Cart_shift(comm_cart, i, disp)
# TODO: replace -1 with MPI.PROC_NULL
@test all( ((neighbors .>= 0) .& (neighbors .< nnodes)) .| (neighbors .== -1) )

This comment has been minimized.

Copy link
@samo-lin

samo-lin Jan 14, 2019

Author

Can you tell me what I need to do to get MPI_PROC_NULL available as MPI.PROC_NULL? Adding MPI_PROC_NULL to deps/gen_constants.f90 was somehow not enough...

This comment has been minimized.

Copy link
@JaredCrean2

JaredCrean2 Jan 14, 2019

Contributor

Most constants show up as "MPI.MPI_CONST_NAME", (which is a bit redundent). MPI.COMM_WORLD and the MPI_Op constants are the exceptions. Can you check deps/src/compile-time.jl for MPI_PROC_NULL? If it is there, it should be available for use.

This comment has been minimized.

Copy link
@samo-lin

samo-lin Jan 14, 2019

Author

Yes, it appears in deps/src/compile-time.jl and MPI.MPI_PROC_NULL works.

@samo-lin

This comment has been minimized.

Copy link
Author

samo-lin commented Jan 14, 2019

@barche and @JaredCrean2, could you please review?

@JaredCrean2
Copy link
Contributor

JaredCrean2 left a comment

Looks good overall. Just a few minor changes.

@@ -300,6 +300,55 @@ function Comm_split_type(comm::Comm,split_type::Integer,key::Integer;info::Info=
MPI.Comm(newcomm[])
end

function Dims_create!(nnodes::Integer, ndims::Integer, dims::Vector{Cint})

This comment has been minimized.

Copy link
@JaredCrean2

JaredCrean2 Jan 14, 2019

Contributor

Arrays that get passed directly into MPI can be MPIBufferType{T}, which allows SubArrays and other things.

This comment has been minimized.

Copy link
@samo-lin

samo-lin Jan 14, 2019

Author

OK, I changed it to MPIBuffertype{T} - also, in the other functions.

nnodes, ndims, dims, 0)
end

function Dims_create!(nnodes::Integer, dims::Array{T,N}) where T <: Integer where N

This comment has been minimized.

Copy link
@JaredCrean2

JaredCrean2 Jan 14, 2019

Contributor

It looks like dims can be AbstractArray{T, N}, because you make a copy into a Cint array.

This comment has been minimized.

Copy link
@samo-lin
for dims in ([0,0,0], [0 0 0])
dims_t = typeof(dims)
MPI.Dims_create!(nnodes, dims)
@test typeof(dims) == dims_t #Ensure the type of dims is as before

This comment has been minimized.

Copy link
@JaredCrean2

JaredCrean2 Jan 14, 2019

Contributor

This test is not needed. Julia can't change the type of the argument in the caller scope (it can make a new variable within the function that has a different type, but the variable dims remains unaffected).

This comment has been minimized.

Copy link
@samo-lin

samo-lin Jan 14, 2019

Author

OK, removed

for i in 0:2
neighbors = MPI.Cart_shift(comm_cart, i, disp)
# TODO: replace -1 with MPI.PROC_NULL
@test all( ((neighbors .>= 0) .& (neighbors .< nnodes)) .| (neighbors .== -1) )

This comment has been minimized.

Copy link
@JaredCrean2

JaredCrean2 Jan 14, 2019

Contributor

Most constants show up as "MPI.MPI_CONST_NAME", (which is a bit redundent). MPI.COMM_WORLD and the MPI_Op constants are the exceptions. Can you check deps/src/compile-time.jl for MPI_PROC_NULL? If it is there, it should be available for use.

Sam
@barche

This comment has been minimized.

Copy link
Collaborator

barche commented Jan 15, 2019

Thanks for the PR, just one small remark: the new functions should also be added to deps/CMakeLists.txt and src/win_mpiconstants.jl, that should allow macOS Travis and Appveyor Win64 to pass.

@samo-lin

This comment has been minimized.

Copy link
Author

samo-lin commented Jan 15, 2019

In src/win_mpiconstants.jl it says

# These constants were manually copied from the file mpi.h in the Microsoft
# MPI SDK v7

I suppose MPI_PROC_NULL should be copied there. @barche, could you do that?

@samo-lin

This comment has been minimized.

Copy link
Author

samo-lin commented Jan 18, 2019

I could not find Microsoft MPI SDK v7 on the net; so I looked at mpi.h from Microsoft MPI v10.0 and took the value from there to copy it into src/win_mpiconstants.jl.

@JaredCrean2

This comment has been minimized.

Copy link
Contributor

JaredCrean2 commented Jan 18, 2019

Appveyor says that works. One of the OSX CI tests failed, but it has failed previously, so I don't think it is related to this PR. I'll give @barche a chance to take a look, otherwise I will merge this PR this evening.

@barche

This comment has been minimized.

Copy link
Collaborator

barche commented Jan 19, 2019

I agree the macOS failure is not related to this PR, strangely it passes on my machine. Thanks!

@barche barche merged commit db9833b into JuliaParallel:master Jan 19, 2019

1 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
coverage/coveralls Coverage increased (+0.5%) to 84.094%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.