Reduce warnings & Initialise MPI with MPI_Init_thread #47

AlexBuccheri · 2025-10-30T14:35:37Z

This MR does two separate things:

Several commits, removing several gfortran compiler warnings
One commit replacing MPI_INIT with MPI_Init_thread, where I request the maximum threading support

W.r.t. compiler warnings, most of the changes silence false positives resulting from implicit LHS allocation. In these cases, I've just added explicit allocate statements. A couple others are from unused arguments. In one instance, I've removed an argument (see the logger) and in the other I'm assigned a dummy attribute.

W.r.t. replacing MPI_INIT with MPI_Init_thread. There's no obvious reason (to me) not to do this, and if a consumer/client code initialises with anything other than MPI_THREAD_SINGLE, this is required. As such, I initialise with MPI_THREAD_MULTIPLE, which should maximise support.

It is still the client code's responsibility to check what's actually provided by the MPI library, and handle appropriately:

    call MPI_Query_thread(provided)
    if (provided < required) then
      call MPI_Comm_rank(MPI_COMM_WORLD, rank)
      if (rank == 0) write(*, '(a)') 'MPI library threading support is less than required by <MY CODE>'
      call MPI_Abort(MPI_COMM_WORLD, 1, ierr)
    end if
``

aradi

Thanks a lot for the PR! This PR should be split into 2 separate ones:

Compiler warning workarounds
MPI-initialization
as the two features are completely unrelated.

As for the compiler warnings: I am not a big fan of writing workarounds to avoid false positive compiler warnings. I am fine with the explicit allocate() statements (to avoid automatic lhs-(re)allocation), but I would not prefer to incorporate the other changes (the string addition and the placeholder local instance variable.)

For the MPI initialization, see the detailed comment below, I think the solution proposed there would be more elegant and robust.

aradi · 2025-11-02T10:10:00Z

.gitignore

 /_*
 /build
 *.mod
+/cmake-*


How and why are files matching this pattern generated during code development or build? (The .gitignore of the repository should only contain patterns, which are generated during the common development workflow.)

This is a Jetbrains convention for cmake build folder names. It's relatively common, but not defacto. I can drop the commit if you prefer.

aradi · 2025-11-02T10:19:36Z

example/serial-fpp/test_fixtured_module_fpp.F90

+    !> State of global random number
+    integer :: rn = 0


Note, this example is there to demonstrate the usage of module variables to pass initialization information. We should, therefore, explicitly do not create a variable within the type, in order to make the example clearer. (What is the actual reason for adding this (unused) instance variable?)

You've got a type-bound procedure that does not use this.

Happy to roll back the change

aradi · 2025-11-02T10:24:14Z

src/fortuno/checkers/int_template.inc

+    allocate(match(size(value1, dim=1), size(value1, dim=2)))
    match = value1 == value2


Suggested change

allocate(match(size(value1, dim=1), size(value1, dim=2)))

match = value1 == value2

allocate(match(size(value1, dim=1), size(value1, dim=2)))

match(:,:) = value1 == value2

Whenever lhs reallocation is not wanted (because lhs had been already allocated) or not possible (because lhs is not allocatable), the placeholder indices should be used for arrays on the lhs. This makes the intention of assigning into more pronounced.

Doesn't this only occur when the LHS is an allocatable variable being assigned a value whose shape/length doesn’t match its current allocation (like, the LHS not being allocated)? In this case, a shape mismatch is impossible because you have if-statements guarding against it.

But happy to make the change

aradi · 2025-11-02T10:24:50Z

src/fortuno/checkers/int_template.inc

+    allocate(match(size(value1, dim=1)))
    match = value1 == value2


Suggested change

allocate(match(size(value1, dim=1)))

match = value1 == value2

allocate(match(size(value1, dim=1)))

match(:) = value1 == value2

aradi · 2025-11-02T10:25:31Z

example/serial-fpp/test_fixtured_module_fpp.F90


    ! Note: factorial(n) with n > 13 overflows with 32 bit integers
    nn = int(13 * rand) + 1
+    this%rn = nn


This should be removed, as we do not want to use variables within the type instance in this example.

aradi · 2025-11-02T10:30:25Z

src/fortuno/checkers/real_template.inc

+    allocate(match(size(value1, dim=1), size(value1, dim=2)))
    match = is_close_elem(value1, value2, atol=atol, rtol=rtol)


Suggested change

allocate(match(size(value1, dim=1), size(value1, dim=2)))

match = is_close_elem(value1, value2, atol=atol, rtol=rtol)

allocate(match(size(value1, dim=1), size(value1, dim=2)))

match(:,:) = is_close_elem(value1, value2, atol=atol, rtol=rtol)

aradi · 2025-11-02T10:30:42Z

src/fortuno/checkers/real_template.inc

+    allocate(match(size(value2, dim=1), size(value2, dim=2)))
    match = is_close_elem(value1, value2, atol=atol, rtol=rtol)


Suggested change

allocate(match(size(value2, dim=1), size(value2, dim=2)))

match = is_close_elem(value1, value2, atol=atol, rtol=rtol)

allocate(match(size(value2, dim=1), size(value2, dim=2)))

match(:,:) = is_close_elem(value1, value2, atol=atol, rtol=rtol)

aradi · 2025-11-02T10:31:41Z

src/fortuno/cmdapp.f90

+    !     & helpmsg="list of tests and suites to include or to exclude when prefixed with '~' (e.g. " //&
+    !     & "'somesuite ~somesuite/avoidedtest' would run all tests except 'avoidedtest' in the test " //&
+    !     & "suite 'somesuite')")&


Why should we use sting addition, if Fortran has a perfect feature of breaking strings into continuation lines? I don't see the point for this change...

I agree it makes no sense, but GCC with warnings enabled will catch what you currently have. Unlike most of the warning fixes, which affect fortuno's test cases (we don't build by default), this one gets caught by Octopus's CI.

So the point is either client codes are forced into disabling specific warnings or one does a trivial patch upstream. Alternatively, we discussed scoping compiler flags on a per library basis some time ago, but since Cristian's left, this is a non-starter.

aradi · 2025-11-02T10:32:32Z

src/fortuno/cmdapp.f90

+        & helpmsg="list of tests and suites to include or to exclude when prefixed with '~' (e.g. " // &
+        & "'somesuite ~somesuite/avoidedtest' would run all tests except 'avoidedtest' in the test " // &
+        & "suite 'somesuite')")


See comment just above. Let's keep the original form, unless there is a really convincing argument for the change.

aradi · 2025-11-02T10:52:18Z

src/fortuno_mpi/mpienv.f90


-    call MPI_Init(ierror)
-    if (ierror /= 0) error stop "MPI_Init failed in init_mpi_env"
+    call MPI_Init_thread(MPI_THREAD_MULTIPLE, provided)


The problem with this approach is, that if the client is using the high level runner routine execute_mpi_cmd_app(), it has no way to check before the tests start, whether the MPI framework provides the necessary level of thread support. So, it is safer here to use the lowest possible threading level (MPI_THREAD_SINGLE, which must work, if the MPI framework works at all). This is also the level of threading support Fortuno requires.

In order to allow codes to use a more advanced threading support, we should allow clients to initialize the MPI-framework before calling execute_mpi_cmd_app() and pass then the global communicator (as optional argument) to execute_mpi_cmd_app(). Fortuno should then just use the provided communicator without initializing or finalizing the MPI framework. The client would then also be responsible for the finalization of the MPI framework after the call.

it has no way to check before the tests start, whether the MPI framework provides the necessary level of thread support.

How does this prevent the caller from calling execute_mpi_cmd_app , followed by a setup that calls MPI_Query_thread?

Fortuno should then just use the provided communicator without initializing or finalizing the MPI framework. The client would then also be responsible for the finalization of the MPI framework after the call.

As discussed over email, I'm fine with this idea. I'll open a separate PR for it

AlexBuccheri · 2025-11-03T18:16:58Z

For the MPI initialization, see the detailed comment below, I think the solution proposed there would be more elegant and robust.

I'll drop the commit and open a separate PR

…necessarily trip

aradi reviewed Nov 2, 2025

View reviewed changes

AlexBuccheri added 5 commits November 3, 2025 18:46

Add explicit allocation of match such that Wuninitialized does not un…

7232061

…necessarily trip

Explicitly allocate yvals, removing compiler warning

9ba1069

Explicitly allocate from source, removing compiler warning

771530a

Close character string per line, removing compiler warning

dd40534

Remove suiteresults arg from log_summary_, as it was unused.

a8f74c1

AlexBuccheri force-pushed the reduce_warnings branch from 51ec80c to a8f74c1 Compare November 3, 2025 18:47

		allocate(match(size(value1, dim=1), size(value1, dim=2)))
		match = value1 == value2

		allocate(match(size(value1, dim=1)))
		match = value1 == value2

		allocate(match(size(value1, dim=1), size(value1, dim=2)))
		match = is_close_elem(value1, value2, atol=atol, rtol=rtol)

		allocate(match(size(value2, dim=1), size(value2, dim=2)))
		match = is_close_elem(value1, value2, atol=atol, rtol=rtol)

Reduce warnings & Initialise MPI with MPI_Init_thread #47

Are you sure you want to change the base?

Reduce warnings & Initialise MPI with MPI_Init_thread #47

Uh oh!

Conversation

AlexBuccheri commented Oct 30, 2025

Uh oh!

aradi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexBuccheri Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexBuccheri commented Nov 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

AlexBuccheri Nov 3, 2025 •

edited

Loading