Skip to content

Potential issues in M7 for Single Precision#2

Open
plesager wants to merge 2 commits into
hamm7-with-historyfrom
hamm7-fix-sp
Open

Potential issues in M7 for Single Precision#2
plesager wants to merge 2 commits into
hamm7-with-historyfrom
hamm7-fix-sp

Conversation

@plesager
Copy link
Copy Markdown
Owner

Try to identify and fix potential issues when running in SP.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 29, 2026

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 34210999-81fa-44b5-a43f-017352e10633

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch hamm7-fix-sp

Comment @coderabbitai help to get the list of available commands and usage tips.

! The argument is that we miss all value between
! EPS(SP)=~1e-7 and EPS(DP)=~2e-16 when running at SP
!
IF (ZVOL(JL) .GE. ZEPS) THEN !eehol: total volume per mode need to be above treshold to avoid div by zero
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

An idea is to devise a the test independent from the precision, for example by using a fixed lower value of 1e-30 (or just 2e-16 so we are closer to what EPS in DP is currently using)

@plesager plesager changed the title Potential issue in M7 for Single Precision Potential issues in M7 for Single Precision May 29, 2026
@noije
Copy link
Copy Markdown

noije commented May 29, 2026

Also, in module/mo_ham_m7.F90, this line:

zrpav=zeps+0.5_dp*(pm6rp(jl,jk,jm1)+pm6rp(jl,jk,jm2))

does not seem to be safe. With pm6rp in cm, the addition of zeps in SP boils down to a shift by zeps=1.19e-7 cm or ~1 nm.

Comment on lines +154 to +155
presult = 0.5_dp * (1.0_dp + erf(arg / sqrt(2.0_dp)))
ccum = 1.0_dp - presult
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

The ERF function is default in Fortran 2003 and above. We could even use the MKL library with Intel, but that can be left for future improvement.
Using erf here removes all the tests on eps and zmin introduced in that subroutine.

@noije
Copy link
Copy Markdown

noije commented May 29, 2026

In general, with EPSILON()=1.19e-7 in SP, we cannot guarantee any longer that it is used correctly throughout the HAM-M7 code, and we need to check this carefully.

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.

2 participants