Clean up code and add several minor features #750
Conversation
Add set_env.memsmall, memmed, memlarge options To use, will require changes to the env machine files. Most machines will probably not use it. See CICE-Consortium#674.
Update cice.batch.csh and cice.launch.csh to use setup_machparams.csh See CICE-Consortium#650
Update ice_transport_remap and ice_transport_driver to call diagnostic_abort during some errors. See also CICE-Consortium#622
Code cleanup based on -std f2003 and f2008 checks Add -stand f08 to cheyenne_intel debug flags Add -std f2008 to cheyenne_gnu debug flags Code consistent with Fortran 2003 except for use of contiguous in 1d evp code.
Add configuration/scripts/tests/qctest.yml file Update documentation
|
@dabail10, could you have a quick look and approve. I know there is a lot here. Would like to merge today for weekend testing if possible. Thanks! |
|
I'll also have a look. |
|
Thanks @phil-blain. In hindsight, I shouldn't have put in all the code cleanup (especially cleaning trailing blanks) with the other mods. I could split up this PR if others feel it's important. |
There was a problem hiding this comment.
Very nice clean-up in general, thanks a lot for these! A few comments on the commits themselves:
- 58e3b8f
Update/improve debug_blocks output, see #718.- thanks!
- 3b0d98d
Add ICE_MEMUSE cice.settings flag for batch memory use Add set_env.memsmall, memmed, memlarge options To use, will require changes to the env machine files. Most machines will probably not use it. See #674.- thanks!
- 6abb59b
Add setup_machparams.csh to compute batch/launch machine parameters Update cice.batch.csh and cice.launch.csh to use setup_machparams.csh See #650- thanks a lot for that, it's a nice improvement!
- a14eb91
Update subroutine diagnostic_abort which calls print_state Update ice_transport_remap and ice_transport_driver to call diagnostic_abort during some errors. See also #622- see comments inline
- f6d09e4
Update miniconda install information See #547- idem
- a95c35c
Code cleanup based on compile with -Wall Code cleanup based on -std f2003 and f2008 checks Add -stand f08 to cheyenne_intel debug flags Add -std f2008 to cheyenne_gnu debug flags Code consistent with Fortran 2003 except for use of contiguous in 1d evp code.- nice to see we are conformant!
- d3b2294
Remove all trailing blank space with script - e630284
Update the cheyenne env so qc testing works Add configuration/scripts/tests/qctest.yml file Update documentation
Note: this is the output of git log --oneline --reverse --no-decorate upstream/main..apcraig/debug01. As you can see Git parses your commit messages as a single line, because they are missing a blank line between the first line (subject) and the rest (body). It would be nice to follow this convention, as I'm not sure how much the squash merge message generated by GitHub will reformat these messages. It's also a good idea also to limit each line to 70 characters for a nice reading experience on the terminal. (Just my 2 cents ;))
| this_block = get_block(blocks_ice(iblk),iblk) | ||
| write (nu_diag,*) 'istep1, my_task, iblk, cat =', & | ||
| istep1, my_task, iblk, '0' |
There was a problem hiding this comment.
here also we loose this cat = 0 bit of info, but I'm not sure if it was really useful...
There was a problem hiding this comment.
Agree, I don't think this adds much so will not re-add.
| # Download the Miniconda installer to ~/miniconda.sh | ||
| curl -L https://repo.anaconda.com/miniconda/Miniconda3-latest-MacOSX-x86_64.sh -o ~/miniconda.sh | ||
| # Install Miniconda | ||
| bash ~/Downloads/miniconda.sh | ||
| bash ~/miniconda.sh |
There was a problem hiding this comment.
There was a problem hiding this comment.
Just to clarify, I added the "closes" to the PR documentation. I think this is a good place for it and how I usually do it. Interesting to know that similar syntax in the commit message could do the same thing.
| HDO_ocn (nx_block,ny_block,max_blocks), & ! seawater concentration of HDO (kg/kg) | ||
| H2_16O_ocn (nx_block,ny_block,max_blocks), & ! seawater concentration of H2_16O (kg/kg) | ||
| H2_18O_ocn (nx_block,ny_block,max_blocks), & ! seawater concentration of H2_18O (kg/kg) | ||
| Qa_iso (nx_block,ny_block,icepack_max_iso,max_blocks), & ! isotope specific humidity (kg/kg) |
There was a problem hiding this comment.
Did you reintroduce trailing spaces here?
There was a problem hiding this comment.
I just shifted over the () to align better. The blank was added to the middle of the line.
|
I had a quick look through everything and didn't see anything beyond @phil-blain 's comprehensive review. |
|
I fixed the output noted by @phil-blain and ran quick suites on cheyenne to confirm the changes compile and run fine. I also updated Icepack to the latest version which also has similar cleanup. I was testing this branch with that version already, so I think it's all fine. |
| else | ||
| write (nu_diag,*) subname,' istep1, my_task, iblk =', & | ||
| istep1, my_task, iblk | ||
| write (nu_diag,*) subname,' Global block:', this_block%block_id |
There was a problem hiding this comment.
In the old code the global block ID was always printed, here it is only printed if istop and jstop are not both greater than zero, i.e. if we go through the else branch.
I think "Global block" should also be printed by print_state to keep the same amount of info. Sorry I missed that on my first review.
There was a problem hiding this comment.
I'm adding it now. Good idea.
| write (nu_diag,*) 'istep1, my_task, iblk, cat =', & | ||
| istep1, my_task, iblk, n |
There was a problem hiding this comment.
OK, here we will end up with duplicated printing of istep1, my_task, iblk, but we do not loose the category info. That's OK.
Out of curiosity, why not add the category as an argument to diagnostic_abort and print_state ? I'm guessing the answer is "to not have to update all callers of these subroutine", which I think does make sense.
There was a problem hiding this comment.
Correct, we can expand print_state later if we really feel it's needed. As it it, print_state writes out info about all category data. I think adding an output that notes which category is causing problems before calling print_state is probably OK for now.
|
I added global block output to print state and ran a handful of tests on cheyenne to make sure it built and ran. |
* Update/improve debug_blocks output, see CICE-Consortium#718. * Add ICE_MEMUSE cice.settings flag for batch memory use Add set_env.memsmall, memmed, memlarge options To use, will require changes to the env machine files. Most machines will probably not use it. See CICE-Consortium#674. * Add setup_machparams.csh to compute batch/launch machine parameters Update cice.batch.csh and cice.launch.csh to use setup_machparams.csh See CICE-Consortium#650 * Update subroutine diagnostic_abort which calls print_state Update ice_transport_remap and ice_transport_driver to call diagnostic_abort during some errors. See also CICE-Consortium#622 * Update miniconda install information See CICE-Consortium#547 * Code cleanup based on compile with -Wall Code cleanup based on -std f2003 and f2008 checks Add -stand f08 to cheyenne_intel debug flags Add -std f2008 to cheyenne_gnu debug flags Code consistent with Fortran 2003 except for use of contiguous in 1d evp code. * Remove all trailing blank space with script * Update the cheyenne env so qc testing works Add configuration/scripts/tests/qctest.yml file Update documentation * Update Icepack * Clean up some output * fix comments * update print_state output
This is a partial (this file only) cherry-pick of d673e44 (Clean up code and add several minor features (CICE-Consortium#750), 2022-08-15).
Since d673e44 (Clean up code and add several minor features (CICE-Consortium#750), 2022-08-15), the 'rad_to_deg' variable in ice_diagnostics::diagnostic_abort is unused as the conversion from radians to degrees is done in 'print_state'. Remove that local variable as well as the icepack_query_parameters call used to set it.
Subroutines 'print_state' and 'diagnostic_abort' in module ice_diagnostics output almost exactly the same information. Add some output to 'print_state' which was only in 'diagnostic_abort' and refactor the latter to call the former, to reduce code duplication. This is a partial (this file only) cherry-pick of d673e44 (Clean up code and add several minor features (CICE-Consortium#750), 2022-08-15).
Since d673e44 (Clean up code and add several minor features (CICE-Consortium#750), 2022-08-15), the 'rad_to_deg' variable in ice_diagnostics::diagnostic_abort is unused as the conversion from radians to degrees is done in 'print_state'. Remove that local variable as well as the icepack_query_parameters call used to set it.
Since d673e44 (Clean up code and add several minor features (CICE-Consortium#750), 2022-08-15), the 'rad_to_deg' variable in ice_diagnostics::diagnostic_abort is unused as the conversion from radians to degrees is done in 'print_state'. Remove that local variable as well as the icepack_query_parameters call used to set it.
Subroutines 'print_state' and 'diagnostic_abort' in module ice_diagnostics output almost exactly the same information. Add some output to 'print_state' which was only in 'diagnostic_abort' and refactor the latter to call the former, to reduce code duplication. This is a partial (this file only) cherry-pick of d673e44 (Clean up code and add several minor features (CICE-Consortium#750), 2022-08-15).
Since d673e44 (Clean up code and add several minor features (CICE-Consortium#750), 2022-08-15), the 'rad_to_deg' variable in ice_diagnostics::diagnostic_abort is unused as the conversion from radians to degrees is done in 'print_state'. Remove that local variable as well as the icepack_query_parameters call used to set it.
Since d673e44 (Clean up code and add several minor features (CICE-Consortium#750), 2022-08-15), the 'rad_to_deg' variable in ice_diagnostics::diagnostic_abort is unused as the conversion from radians to degrees is done in 'print_state'. Remove that local variable as well as the icepack_query_parameters call used to set it.
Since d673e44 (Clean up code and add several minor features (CICE-Consortium#750), 2022-08-15), the 'rad_to_deg' variable in ice_diagnostics::diagnostic_abort is unused as the conversion from radians to degrees is done in 'print_state'. Remove that local variable as well as the icepack_query_parameters call used to set it.
PR checklist
Short (1 sentence) summary of your PR:
Clean up code and add a few new features
Developer(s):
apcraig
Suggest PR reviewers from list in the column to the right.
Please copy the PR test results link or provide a summary of testing completed below.
Tested with "clean01" in Icepack, https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#e630284d5baa877f93c8ca3003bcb2fa8c27e63f
How much do the PR code changes differ from the unmodified code?
Does this PR create or have dependencies on Icepack or any other models?
Does this PR add any new test cases?
Is the documentation being updated? ("Documentation" includes information on the wiki or in the .rst files from doc/source/, which are used to create the online technical docs at https://readthedocs.org/projects/cice-consortium-cice/. A test build of the technical docs will be performed as part of the PR testing.)
Please provide any additional information or relevant details below:
Update debug blocks output, closes 'debug_blocks' only lists the block distribution on first proc since #602 #718
Add memory use batch setting for machines, closes Adding options (
cice.setup -s) for amount of memory needed #674Migrate batch task/thread computations to a new file, setup_mach_params.csh, closes Document MPI/OpenMP/nodes distribution done in
cice.batch.csh#650Update subroutine diagnostic_abort which calls print_state for a failing point, added to departure points errors and a few other places, closes Add debug output on aborts #622
Update the miniconda install information so we no longer download to ~/Downloads on MacOS, closes conda MacOSX issues and Github actions #547
Check conformation with f2003 and f2008. We conform to 2003 except use of "contiguous" in 1d evp solver. That is f2008. Also compiled with -Wall and removed some unused variables. Closes Compiler standard conventions check #455
Remove trailing blank space via a automated script.
Fixed automated qc testing on cheyenne for prod_suite testsuite. Was not completing the qc test completely. Needed to create a conda env to install utilities for the qc tool. The old python env we were using was deprecated recently. The new NCAR npl conda env didn't work because it broke the CICE compile. pip install didn't work for compatibility reasons not well understood. So I added a qctest.yml file to configuration/scripts/tests and installed it in my env by default. This seems to fix the problem for me, but use of the automated qc testing defined in the prod_suite may cause problems for other users and on other machines.