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

Quickbuild mpif08 #562

Merged
merged 25 commits into from
Nov 7, 2023
Merged

Quickbuild mpif08 #562

merged 25 commits into from
Nov 7, 2023

Conversation

hkershaw-brown
Copy link
Member

@hkershaw-brown hkershaw-brown commented Oct 23, 2023

Description:

Added an mpif08 option to quickbuild.sh for model quickbuilds.

I've added mpi as an option for the 1st argument to quickbuild.sh. This is because it seemed likely that someone might specify ./quickbuild.sh mpi if they can do ./quickbuild.sh mpif08

The default is still mpi.

The GSI converter uses mpi, but its own mpi calls (not mpif08)
https://github.com/NCAR/DART/blob/main/observations/obs_converters/GSI2DART/mpisetup.f90
I have left this alone for now.

Fixes issue

Fixes #261, fixes #551, fixes #497

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?

Tests

Building, with mpi, mpif08, nompi
./quickbuild.sh help To print usage message.
will test on Derecho with the various compilers and mpi libraries once derecho is a back up.

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

@hkershaw-brown hkershaw-brown marked this pull request as draft October 23, 2023 19:14
Copy link
Collaborator

@nancycollins nancycollins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

everything looks fine except i was confused by the "core" lists in the buildfunctions.sh for the mpi vs nompi cases. it's probably how it should be but it looks strange on first read.


if [ "$mpisrc" == "mpi" ]; then

core=${core//$nullmpi/}
core=${core//$nullwin/}
core=${core//$mpif08/}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks backwards to me - is it just the labels? mpi includes the nullmpi version, and below the #nompi version includes the mpi modules?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does look backwards with the // at the beginning, but if you believe me:

core=${core//$nullmpi/}

replace all matches (// ) of$nullmpi from $core with nothing ('/ ')

so for mpi, remove null, nullwin, mpif08, mpif08, no_cray_winf08

@hkershaw-brown hkershaw-brown added the release! bundle with next release label Oct 26, 2023
Copy link
Collaborator

@nancycollins nancycollins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good.

@hkershaw-brown hkershaw-brown marked this pull request as draft November 6, 2023 13:45
@hkershaw-brown
Copy link
Member Author

needs converters

nompi
mpi - this is for GSI only (no mpif08 code for that converter)
added note on quickbuild.sh help to getting started
@hkershaw-brown hkershaw-brown marked this pull request as ready for review November 6, 2023 14:54
Copy link
Contributor

@mjs2369 mjs2369 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/NCAR/DART/blob/6c6ac535ff2a75d2b1831b5e414570cf370a3a66/build_templates/buildconvfunctions.sh#L45C1-L55C2

In the function print_usage in buildconvfunctions.sh, it should echo ./quickbuild.sh instead of buildconverter.sh

Also, is line 52 (copied below) true? I don't think that we can build individual programs with obs_converters
echo " buildconverter.sh [program] : build a single program"

Attempting to run ./quickbuild.sh filter while in the work dir for an obs converter results in "ERROR: unknown program filter"

quickbuild.sh clean : clean the build
quickbuild.sh help : print help message

quickbuild.sh [nompi/nompi/mpif08] [program] : optional arguments
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
quickbuild.sh [nompi/nompi/mpif08] [program] : optional arguments
quickbuild.sh [mpi/nompi/mpif08] [program] : optional arguments

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in 3af2899

@hkershaw-brown
Copy link
Member Author

https://github.com/NCAR/DART/blob/6c6ac535ff2a75d2b1831b5e414570cf370a3a66/build_templates/buildconvfunctions.sh#L45C1-L55C2

In the function print_usage in buildconvfunctions.sh, it should echo ./quickbuild.sh instead of buildconverter.sh

Also, is line 52 (copied below) true? I don't think that we can build individual programs with obs_converters echo " buildconverter.sh [program] : build a single program"

Attempting to run ./quickbuild.sh filter while in the work dir for an obs converter results in "ERROR: unknown program filter"

Fixed buildconverter.sh -> quickbuild.sh

The converter quickbuilds will build a single program, but it has to be one of the converter programs. Do you think it would be less confusing to just have the docs say run ./quickbuild.sh help in a work directory to see the options?

@mjs2369
Copy link
Contributor

mjs2369 commented Nov 6, 2023

The converter quickbuilds will build a single program, but it has to be one of the converter programs. Do you think it would be less confusing to just have the docs say run ./quickbuild.sh help in a work directory to see the options?

I think that is already covered in the docs, unless you mean that running "./quickbuild help" will show the options for possible single programs that can be built for the given work dir, which it currently doesn't do

@hkershaw-brown
Copy link
Member Author

The converter quickbuilds will build a single program, but it has to be one of the converter programs. Do you think it would be less confusing to just have the docs say run ./quickbuild.sh help in a work directory to see the options?

I think that is already covered in the docs, unless you mean that running "./quickbuild help" will show the options for possible single programs that can be built for the given work dir, which it currently doesn't do

To address #497, I put in the the usage for quickbuild.sh for lorenz_63.
but it seems like this is confusing for converter builds. What were you thinking for #497 ?

@mjs2369
Copy link
Contributor

mjs2369 commented Nov 6, 2023

To address #497, I put in the the usage for quickbuild.sh for lorenz_63.
but it seems like this is confusing for converter builds. What were you thinking for #497 ?

I really like the additions to the usage for quickbuild.sh for lorenz_63 and think it fixes 497 well. I don't think it's confusing for the obs converters; if they try to use the mpi options it will fail and from there they can use the help command.

But we could maybe add something like "not for obs converters" to this line here

Copy link
Contributor

@mjs2369 mjs2369 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the compiling-dart doc is great! Ready to merge

hkershaw-brown and others added 6 commits November 7, 2023 14:45
Fixes for remaining documentation warnings in #546
remove loop from Mersenne twister test
doc fix: rename assim_mod_mod docs to match the module
fix: sync filter_mod.dopplerfold with filter_mod
@hkershaw-brown hkershaw-brown merged commit 1244b87 into main Nov 7, 2023
3 checks passed
@hkershaw-brown hkershaw-brown deleted the quickbuild-mpif08 branch November 7, 2023 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release! bundle with next release
Projects
None yet
5 participants