OpenCMISS / iron

Source code repository for OpenCMISS-Iron
9 stars 62 forks source link

Make generate bindings script work with Python 3 #10

Closed hsorby closed 8 years ago

rondiplomatico commented 8 years ago

The reason for this change was that both andre and hugh experienced errors with MPI_LOGICAL, see also https://github.com/OpenCMISS/manage/issues/44 we've tested the update on many other machines and they didnt complain. so thus far we have a +2 machines working - is there more that needs to be accounted for?

this has nothing to do with python directly, this is only fixing up the second line where this change needs to be done - happens to be in the same pull request.

also, as to the old cm repo - i consider it deprecated as using the new iron repo has everything from cm plus all the CMake goodness. do you think this fix should also be in cm?

chrispbradley commented 8 years ago

They may have had errors but changing from MPI_LOGICAL to MPI_INTEGER is not the solution. What errors did they have. The old cm repo is not deprecated and is the current main repo until we officially switch over. Changes should go into it and then pulled through. But, no, the MPI change should go into cm as it needs to be reverted as it is incorrect. Fortran does not define logical to be the same as integer and it is compiler and system dependent how it is implemented and so whilst it may work for some it may not work for others.

hsorby commented 8 years ago

From the documentation

All MPI objects (e.g., MPI_Datatype, MPI_Comm) are of type INTEGER in Fortran.

So the arguments should be integers and not logical. Thus other changes are required.

chrispbradley commented 8 years ago

I'm not sure what documentation that is but the only documentation that matters is the MPI standard. That defines the MPI_LOGICAL for Fortran LOGICAL data types. The data is of type LOGICAL and so the changes to MPI_INTEGER need to be reverted to MPI_LOGICAL. What are the errors you are getting? There will be another problem/solution than changing the data type.

hsorby commented 8 years ago

So no, the method specification states that (in fortran) the argument type must be integer.

https://www.open-mpi.org/doc/v1.8/man3/MPI_Allgather.3.php http://www.mpich.org/static/docs/v3.2/www3/MPI_Allgather.html

The data type must be changed to integer.

chrispbradley commented 8 years ago

The INTEGER in the references above is referring to the type of the predefined MPI constant like MPI_LOGICAL i.e., MPI_LOGICAL is of an integer type and will have an integer value (as opposed to, say, character). The actual value (rather than the type) you need to pass depends on the data type of the send and receive buffers. For the call above the send and receive buffers are of type LOGICAL and so you tell the MPI library this by passing the predefined constant MPI_LOGICAL. You only use MPI_INTEGER if the send and recieve buffers are of type INTEGER (which they aren't).

hsorby commented 8 years ago

This doesn't work, as the documentation states, the only acceptable input is of type integer. When you have a compiler that cares about parameter matching trying to use anything else other than integer is an error. The user is not free to define the type in fortran.

chrispbradley commented 8 years ago

Yes, the data type parameter is of type integer. MPI_LOGICAL and MPI_INTEGER are INTEGER constants. You can not use, say, a real for the data type parameter in the call. However, the value of that integer parameter indicates the type of the send and recieve buffers (localConvergered and globalConverged in the call above). Say you wanted to transfer 10 doubles (8 bytes each) vs 10 reals (4 bytes each). You would tell MPI you have 10 items to send in the send count (2nd parameter in the call above). However you also need to say they are doubles so that MPI knows it has to send 80 total bytes rather than 40 total bytes for reals. In the call above we are sending buffers of LOGICAL type and so we use the predefined integer constant MPI_LOGICAL to indicate this. We are not sending buffers if INTEGER type and so we don't use MPI_INTEGER.

rondiplomatico commented 8 years ago

chris, so we had a solution that seemed to agree with documentation and compiled on all our tested platforms including new ones. i see the point that you want/need MPI_LOGICAL, but that does not provide a solution as to why newer compilers seem to disagree with you on that. so what exactly needs to be done here if NOT replacing MPI_LOGICAL by MPI_INTEGER?

chrispbradley commented 8 years ago

What you have is incorrect MPI. It does not agree with the documentation (people seem to be confusing type with value of the type). It is thus not a solution. There are a few MPI books in my office to borrow if the explation above isn't working (or google an MPI tutorial?)? What is the error you have? Both MPI_LOGICAL and MPI_INTEGER are of type INTEGER and so there shouldn't be a type mismatch error?

rchristie commented 8 years ago

Was this (re)definition of MPI_LOGICAL causing the problem:

LOGICAL :: boundaryNode,boundaryConverged(30),localConverged,MPI_LOGICAL
chrispbradley commented 8 years ago

That would do it!

chrispbradley commented 8 years ago

So is anybody going to revert the MPI change???