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

Adds stand alone test_MOM_EOS and time_MOM_EOS #516

Merged

Conversation

adcroft
Copy link
Member

@adcroft adcroft commented Nov 3, 2023

  • Added simple single-thread program to invoke EOS unit tests
  • Added not-as-simple program to time the cost of calculate_density() and calculate_spec_vol() for both scalar and array APIs
    • Placed in new directory config_src/drivers/timing_tests/
  • Renamed MOM_unit_test_driver.F90 to test_MOM_file_parser.F90
  • Updated .testing/Makefile
    • Added list of programs in config_src/drivers/unit_tests - These are added to BUILDS if DO_UNIT_TESTS is not blank. (DO_UNIT_TESTS was an existing macro but it might be unneeded) - These programs are compiled with code coverage
    • Added list of programs in config_src/drivers/timing_tests
      • These programs are compiled with optimization and no coverage
    • Fixed rule for building UNIT_EXECS (which did not re-build properly because the central Makefile was trying to model the dependencies even though those dependencies are in the build/unit/Makefile.dep)
    • Added convenient targets build.unit, run.unit, build.timing, run.timing
  • Timing tests currently time a loop over 1000 calls (so that the resolution of the CPU timer is not too large) and 400 samples to collect statistics on timings. On gaea c5 this takes about 10 seconds.
    • The results are written to stdout in json.
  • Added placeholder build and run of timing_tests to GH workflow.
    • Enabled for [push,pull_request]
    • We probably will not be able to use timings from GH but I still want to exercise the code we know the timing programs aren't broken by a commit.
  • Also added driver for string_functions_unit_tests

Copy link

codecov bot commented Nov 3, 2023

Codecov Report

Merging #516 (b6a5976) into dev/gfdl (0f2a69d) will decrease coverage by 0.01%.
The diff coverage is 28.88%.

❗ Current head b6a5976 differs from pull request most recent head aa22fd8. Consider uploading reports for the commit aa22fd8 to get more accurate results

@@             Coverage Diff              @@
##           dev/gfdl     #516      +/-   ##
============================================
- Coverage     37.45%   37.44%   -0.01%     
============================================
  Files           270      270              
  Lines         79692    79722      +30     
  Branches      14823    14824       +1     
============================================
+ Hits          29849    29855       +6     
- Misses        44308    44331      +23     
- Partials       5535     5536       +1     
Files Coverage Δ
...ig_src/drivers/unit_tests/test_MOM_file_parser.F90 92.00% <100.00%> (ø)
...ameterizations/lateral/MOM_mixed_layer_restrat.F90 70.03% <ø> (ø)
src/equation_of_state/MOM_EOS.F90 54.36% <0.00%> (-0.89%) ⬇️
src/framework/MOM_error_handler.F90 56.86% <40.74%> (-2.91%) ⬇️

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

Copy link
Member

@Hallberg-NOAA Hallberg-NOAA left a comment

Choose a reason for hiding this comment

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

I have examined the Fortran90 portions of this commit, and they all make sense to me. I would approve the changes to the 8 Fortran90 files in this commit (after one minor change to some indents), but I would like to have someone else (@marshallward?) review the new or revised Makefile and .py and .yml files in this PR.

Copy link
Member

@marshallward marshallward left a comment

Choose a reason for hiding this comment

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

Python script needs a PEP8 update, but otherwise looks fine.

I noted some Fortran style fixes.

There are some conflicts from the recent Makefile changes.

src/equation_of_state/MOM_EOS.F90 Outdated Show resolved Hide resolved
src/equation_of_state/MOM_EOS.F90 Outdated Show resolved Hide resolved
src/equation_of_state/MOM_EOS.F90 Outdated Show resolved Hide resolved
config_src/drivers/timing_tests/time_MOM_EOS.F90 Outdated Show resolved Hide resolved
config_src/drivers/timing_tests/time_MOM_EOS.F90 Outdated Show resolved Hide resolved
config_src/drivers/timing_tests/time_MOM_EOS.F90 Outdated Show resolved Hide resolved
config_src/drivers/timing_tests/time_MOM_EOS.F90 Outdated Show resolved Hide resolved
config_src/drivers/unit_tests/test_MOM_EOS.F90 Outdated Show resolved Hide resolved
- Added simple single-thread program to invoke EOS_unit_tests.F90
- Added not-as-simple program to time the cost of calculate_density()
  and calculate_spec_vol() for both scalar and array APIs
  - Placed in new directory config_src/drivers/timing_tests/
- Renamed MOM_unit_test_driver.F90 to test_MOM_file_parser.F90
- Updated .testing/Makefile
  - Added list of programs in config_src/drivers/unit_tests
    - These are added to BUILDS if DO_UNIT_TESTS is not blank.
      (DO_UNIT_TESTS was an existing macro but it might be uneeded)
    - These programs are compiled with code coverage
  - Added list of programs in config_src/drivers/timing_tests
    - These programs are compiled with optimization and no coverage
  - Fixed rule for building UNIT_EXECS (which did not re-build properly
    because the central Makefile was trying to model the dependencies
    even though those dependencies are in the build/unit/Makefile.dep)
  - Added convenient targets build.unit, run.unit, build.timing, run.timing
- Timing tests currently time a loop over 1000 calls (so that the resolution
  of the CPU timer is not too large) and 400 samples to collect statistics
  on timings. On gaea c5 this takes about 10 seconds.
  - The results are written to stdout in json.
- Added placeholder build and run of timing_tests to GH workflow.
  - Enabled for [push,pull_request]
  - We probably will not be able to use timings from GH but I still want
    to exercise the code we know the timing programs aren't broken by a
    commit.
- Also added driver for string_functions_unit_tests
Copy link
Member

@Hallberg-NOAA Hallberg-NOAA left a comment

Choose a reason for hiding this comment

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

This is all looking good to me now, and it also looks like Marshall's concerns have been addressed.

@Hallberg-NOAA
Copy link
Member

This has passed pipeline testing at gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/21302.

@Hallberg-NOAA Hallberg-NOAA merged commit b15a9d4 into NOAA-GFDL:dev/gfdl Nov 10, 2023
10 checks passed
@adcroft adcroft deleted the add-stand-alone-unit-perf-tests branch October 24, 2024 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants