fix: bounds error in mpi{f08} utilities when make_copy_before_broadcast = .true.

[hkershaw-brown]

Description:

When copy before broadcast is set to true, array is copied into a temporary variable. This variable was a different size to array and so gave an out-of-bounds error when doing the copy.

This pull request uses the itemcount to right-size the array copy (and copy back after broadcast).

Note you have to edit the code to switch on the mpi namelist to select "copy before broadcast" so I do not think users were hitting this, but it did trip me up debugging openmpi issues in openmpi in various containers #1118.

Fixes issue

fixes #1168

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update

Documentation changes needed?

• My change requires a change to the documentation.
  • I have updated the documentation accordingly.

Tests

Please describe any tests you ran to verify your changes.
Running lorenz_96 with mpi (and mpi_utitiles namelist) making a copy before broadcast.

Checklist for merging

• Updated changelog entry
• Documentation updated
• Update conf.py

Checklist for release

• Merge into main
• Create release from the main branch with appropriate tag
• Delete feature-branch

Testing Datasets

• Dataset needed for testing available upon request
• Dataset download instructions included
• No dataset needed

[mjs2369]

@hkershaw-brown

This is a clean fix, but why even keep make_copy_before_broadcast and make_copy_before_sendrecv in the code? Multiple places state it is for an old bug that should be fixed now, so do we know if this is the case?

DART/assimilation_code/modules/utilities/mpi_utilities_mod.f90

Lines 128 to 131 in c227001

	! make local array copy for send/recv/bcast. was needed on an old, buggy version
	! of the mpi libs but seems unneeded now.
	logical :: make_copy_before_sendrecv = .false. ! should not be needed; .true. is very slow
	logical :: make_copy_before_broadcast = .false. ! should not be needed; .true. is very slow

I know sometimes we are a little hesitant to change the namelists as to not break users' experiments, but especially if you have to edit the code to even use the mpi namelist, I don't think it would really be an issue

side note make_copy_before_broadcast is not in the docs, but make_copy_before_sendrecv is:
https://docs.dart.ucar.edu/en/latest/assimilation_code/modules/utilities/mpi_utilities_mod.html#:~:text=leave%20this%20true.-,make_copy_before_sendrecv,-logical

[hkershaw-brown]

@hkershaw-brown

This is a clean fix, but why even keep make_copy_before_broadcast and make_copy_before_sendrecv in the code? Multiple places state it is for an old bug that should be fixed now, so do we know if this is the case?

DART/assimilation_code/modules/utilities/mpi_utilities_mod.f90

Lines 128 to 131 in c227001

	! make local array copy for send/recv/bcast. was needed on an old, buggy version
	! of the mpi libs but seems unneeded now.
	logical :: make_copy_before_sendrecv = .false. ! should not be needed; .true. is very slow
	logical :: make_copy_before_broadcast = .false. ! should not be needed; .true. is very slow

I know sometimes we are a little hesitant to change the namelists as to not break users' experiments, but especially if you have to edit the code to even use the mpi namelist, I don't think it would really be an issue

side note make_copy_before_broadcast is not in the docs, but make_copy_before_sendrecv is: https://docs.dart.ucar.edu/en/latest/assimilation_code/modules/utilities/mpi_utilities_mod.html#:~:text=leave%20this%20true.-,make_copy_before_sendrecv,-logical

Yeah this is dart all over, why are we keeping things like this?
It tripped me up when debugging a different mpi problem, and I had to fix it to solve my problem.

I agree with you, it is better to have less code, and the code we have should work.
How about as part of your project looking at mpi in DART, flag things like this (and #1143). Set up tests + docs and update the mpi modules?

[mjs2369]

Yeah this is dart all over, why are we keeping things like this? It tripped me up when debugging a different mpi problem, and I had to fix it to solve my problem.

I agree with you, it is better to have less code, and the code we have should work. How about as part of your project looking at mpi in DART, flag things like this (and #1143). Set up tests + docs and update the mpi modules?

Great idea, I'll set up a new issue for the future mpi updates, where I can add info about these issues and add on as I go.

I'll approve this PR