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.

.gitignore

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

Yes please. That is somewhat unfortunate, that compilers complain about unused this even in extending types, where you can not change the signature of the type bound procedure...

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

You are right, theoretically, it only makes a difference if the LHS is not allocated or is allocated with a mismatching shape. However, I adopted the convention from DFTB+, that placeholder indices on the LHS are consistently used to indicate the intent of the assignment:

no placeholder indices: I wish to trigger automatic reallocation if necessary

placeholder indices: I want to make sure, that no reallocation is triggered (either because the array is static or because it has been allocated with the matchin shape)
So it is semantically consistent, easier to understand the intention and helps to avoid bottlenecks by triggering automatic reallocation by mistake within a loop or similar.

I think, this convention would be also useful for Fortuno.

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.

I have checked gfortran 15.2:

gfortran -Dfortuno_EXPORTS -I[...]fortuno/src/modules -I[...]fortuno/src/fortuno/checkers -I[...]fortuno/include -fcheck=all -std=f2018 -Wall -pedantic -g -Jmodules -fPIC -c [...]fortuno/src/fortuno/cmdapp.f90 -o CMakeFiles/fortuno.dir/fortuno/cmdapp.f90.o

which does not produce any warnings in cmdapp.f90. Do you use any other flags, which trigger this warning? Or does the warning appear in your case with the most recent gfortran? (In case, they fixed it in recent gfortran versions, I would probably tend to keep the original formulation. I kind consider Fortuno also as a demonstration of best practices when programming in modern Fortran.)

OK, I also checked GFortran 13.2 now and I do not get any warnings for cmdapp.f90 with the -std=f2018 -Wall -pedantic flag combination. Do you use an older compiler (which could cause other troubles due to compiler bugs) or a different warning level setting?

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 · 2025-11-26T11:45:52Z

I've just tested the build process with GFortran 15.2 using the code from the main branch. I get with -Wall -pedantic -std=f2018 only a single warning about an unused dummy argument in consolelogger.f90. (Which we could easily fix). No other warnings anywhere else. So it seems, that the GFortran folks fixed the warning artifacts in the compiler already. I am wondering, where does this leave us?

aradi · 2025-11-26T12:49:53Z

@AlexBuccheri I have added in my cleanup PR (#46) a fix, which also eliminates the unused dummy argument problem in the console logger. That code runs with GFortran 15.2 without any warnings with the settings above. Would that code version something you could live with?

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

hmenke mentioned this pull request Nov 21, 2025

Possibility to use MPI_Init_thread #49

Closed

		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!

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!

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!

aradi commented Nov 26, 2025

Uh oh!

aradi commented Nov 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

AlexBuccheri Nov 3, 2025 •

edited

Loading