Skip to content

Add threading test to capgen_test; codee format test/capgen_test/*.F90 - #722

Merged
mkavulich merged 5 commits into
NCAR:developfrom
climbfuji:feature/capgen_threading_test
Mar 23, 2026
Merged

Add threading test to capgen_test; codee format test/capgen_test/*.F90#722
mkavulich merged 5 commits into
NCAR:developfrom
climbfuji:feature/capgen_threading_test

Conversation

@climbfuji

@climbfuji climbfuji commented Mar 2, 2026

Copy link
Copy Markdown
Collaborator
  1. Add threading test to capgen_test. This is exercised with ctest without any further changes, and it passes. I added a comparison of all averages to the compare_data call, which compares the omp1 and omp2 test runs. This is commit 900069b. While testing this, @peverwhee and I uncovered a bug (uninitialized variable, see Add threading test to capgen_test; codee format test/capgen_test/*.F90 #722 (comment)) that I had to fix.
  2. Update Codee to 2025.4.8 and simplify .github/workflows/fortran-formatting.yaml (there was an actual error using the workflow that needed to be fixed). This is commit a76f82c
  3. Codee format test/capgen_test/*.F90. This is commit 24223ff

User interface changes?: No

Issues: Working toward #721

Testing: all pass
test removed: n/a
unit tests: n/a
system tests: existing capgen test now run with OMP_NUM_THREADS=1 and OMP_NUM_THREADS=2
manual testing: tested manually on my dev machine with spack-stack-2.1.0

@climbfuji
climbfuji requested review from a team and gold2718 as code owners March 2, 2026 15:56
@climbfuji
climbfuji force-pushed the feature/capgen_threading_test branch from a9e77fa to a76f82c Compare March 2, 2026 17:04
@climbfuji climbfuji changed the title Add threading test to test/capgen_test Add threading test to capgen_test; codee format test/capgen_test/*.F90 Mar 2, 2026

@dustinswales dustinswales left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for adding these features.
I like how extensible the ctest was in this case!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is slick!

@peverwhee

Copy link
Copy Markdown
Collaborator

hi @climbfuji i'm working through this now, but thought I'd mention that I ran the unit tests on derecho and found that the new thread tests fail (answer differences) with ifx (but pass with ifort). Not sure if this is critical. If we determine that we'll proceed without fixing that now, I will make an issue.

@climbfuji

Copy link
Copy Markdown
Collaborator Author

@peverwhee That's interesting. I thought I had tested on my laptop with ifx. Let me go back and retest. If it's only on Derecho, then we'll want to file an issue and fix it later.

@climbfuji

Copy link
Copy Markdown
Collaborator Author

@peverwhee That's interesting. I thought I had tested on my laptop with ifx. Let me go back and retest. If it's only on Derecho, then we'll want to file an issue and fix it later.

I figured that I tested with gcc on my laptop earlier. I can reproduce your error with oneapi@2025.3.0. The results are identical between omp1 and omp2 with oneapi@2025.3.0, but what is concerning is that the values are vastly different between gfortran and ifx - not just a bit. I will need to investigate what is going on here.

@peverwhee

peverwhee commented Mar 12, 2026

Copy link
Copy Markdown
Collaborator

@peverwhee That's interesting. I thought I had tested on my laptop with ifx. Let me go back and retest. If it's only on Derecho, then we'll want to file an issue and fix it later.

I figured that I tested with gcc on my laptop earlier. I can reproduce your error with oneapi@2025.3.0. The results are identical between omp1 and omp2 with oneapi@2025.3.0, but what is concerning is that the values are vastly different between gfortran and ifx - not just a bit. I will need to investigate what is going on here.

Thanks for the update, @climbfuji

If it helps, it also fails with oneapi@2025.2.1. Let me know if you want a second pair of eyes. Since we haven't extensively tested the generated caps with ifx, it could be something somewhat unrelated to threading that's popping up

@climbfuji

Copy link
Copy Markdown
Collaborator Author

@peverwhee That's interesting. I thought I had tested on my laptop with ifx. Let me go back and retest. If it's only on Derecho, then we'll want to file an issue and fix it later.

I figured that I tested with gcc on my laptop earlier. I can reproduce your error with oneapi@2025.3.0. The results are identical between omp1 and omp2 with oneapi@2025.3.0, but what is concerning is that the values are vastly different between gfortran and ifx - not just a bit. I will need to investigate what is going on here.

Thanks for the update, @climbfuji

If it helps, it also fails with oneapi@2025.2.1. Let me know if you want a second pair of eyes. Since we haven't extensively tested the generated caps with ifx, it could be something somewhat unrelated to threading that's popping up

I am fairly certain that it's something else. Let me dig a little more later, maybe it has to do with the flags, uninitialized data, ...

@climbfuji
climbfuji force-pushed the feature/capgen_threading_test branch from 3a38b76 to 419a6c2 Compare March 13, 2026 13:41
@climbfuji

climbfuji commented Mar 13, 2026

Copy link
Copy Markdown
Collaborator Author

@peverwhee Indeed, I found an uninitialized variable (cind). After setting it to a safe and reasonable value of 1, the results match for gcc@13 and oneapi@2025.3.0. Interestingly, the value now matches what ifx produced prior to the bug fix, i.e. ifx must have initialized cind to 1. Please try again on your end.

The bug fix is in commit 419a6c2

@climbfuji climbfuji self-assigned this Mar 13, 2026
@climbfuji climbfuji added the capgen bugs, requests, etc. that involve ccpp_capgen label Mar 13, 2026

@peverwhee peverwhee left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

thanks @climbfuji ! tests pass for me with both ifx and gnu on derecho.

I do have a couple of codee questions.

Comment thread test/capgen_test/temp_adjust.F90 Outdated
Comment thread test/capgen_test/temp_adjust.F90
Comment thread test/capgen_test/temp_calc_adjust.F90 Outdated
Comment thread test/capgen_test/test_host_mod.F90
…st.F90 and test/capgen_test/temp_calc_adjust.F90 to test Fortran parser; try to prevent Codee from formatting these blocks
@climbfuji
climbfuji requested a review from peverwhee March 13, 2026 23:47
@climbfuji

Copy link
Copy Markdown
Collaborator Author

@peverwhee Any chance you can re-review this, please?

@peverwhee peverwhee left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

thanks for looking into the Codee stuff - good to know that it can be turned off pretty easily.

@climbfuji

Copy link
Copy Markdown
Collaborator Author

@mkavulich I think this can be merged, it's got 2 approvals and it's been out there for three weeks.

@mkavulich mkavulich left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Procedural approval for NEPTUNE contingent

@mkavulich
mkavulich merged commit fbb5b30 into NCAR:develop Mar 23, 2026
15 checks passed
climbfuji added a commit to climbfuji/ccpp-framework that referenced this pull request Apr 3, 2026
….F90` (NCAR#722)

1. Add threading test to `capgen_test`. This is exercised with `ctest`
without any further changes, and it passes. I added a comparison of all
averages to the `compare_data` call, which compares the `omp1` and
`omp2` test runs. This is commit
NCAR@900069b.
While testing this, @peverwhee and I uncovered a bug (uninitialized
variable, see
NCAR#722 (comment))
that I had to fix.
2. Update Codee to 2025.4.8 and simplify
`.github/workflows/fortran-formatting.yaml` (there was an actual error
using the workflow that needed to be fixed). This is commit
NCAR@a76f82c
3. Codee format `test/capgen_test/*.F90`. This is commit
NCAR@24223ff

User interface changes?: No

Issues: Working toward NCAR#721

Testing: all pass
  test removed: n/a
  unit tests: n/a
system tests: existing capgen test now run with `OMP_NUM_THREADS=1` and
`OMP_NUM_THREADS=2`
manual testing: tested manually on my dev machine with spack-stack-2.1.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

capgen bugs, requests, etc. that involve ccpp_capgen

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants