Deprecating mixed precision inner product and kgo changes#501
Deprecating mixed precision inner product and kgo changes#501Matthew Walker (mattatmet) wants to merge 5 commits into
Conversation
github-actions
Bot
added
the
cla-required
Matthew Walker (mattatmet)
added
Linked Core
github-actions
Bot
added
cla-signed
Alistair Pirrie (mo-alistairp)
left a comment
Alistair Pirrie (mo-alistairp)
left a comment
There was a problem hiding this comment.
Happy with these changes from a TCD team/PSyclone perspective - I'm definitely happy to see PSyKAl-lite code be removed, thanks for doing that. I've left one comment in science/gungho/integration-test/cma_test/cma_test_algorithm_mod.x90 but otherwise happy.
| @@ -390,11 +389,11 @@ contains | |||
|
|
|||
| ! Calculate differences | |||
| call invoke( X_minus_Y( dx, x1, x2 ) ) | |||
| call invoke_rdouble_X_innerproduct_X(tmp, dx) | |||
| call invoke( X_innerproduct_X(tmp, dx) ) | |||
There was a problem hiding this comment.
This should be merged into the invoke above
There was a problem hiding this comment.
Thanks for this suggestion Alistair. I've made and tested that change now.
Matthew Walker (mattatmet)
requested review from
a team,
Richard Hill (HengistPodd),
cjohnson-pi,
iboutle,
Matt Shin (matthewrmshin),
Steven Sandbach (ss421) and
thomasmelvin
as code owners
|
This will very likely need a linked jedi PR, I will link it once ready. |
DanStoneMO
added
the
Linked Jedi
DanStoneMO
left a comment
DanStoneMO
left a comment
There was a problem hiding this comment.
lfric-jedi PR is now live at: https://github.com/JCSDA-internal/lfric-jedi/pull/1281
iboutle
left a comment
iboutle
left a comment
There was a problem hiding this comment.
I'll defer to Tom M on whether we're happy with changing the precision of these solver calculations. I'm slightly nervous that they were done in double precision for a reason (and we also do it in Endgame), but also understand that reason may no longer be valid given the different solver setup we have. I'm happy it should be scientifically neutral as long as it's stable. Can you at least please check that the job lfric_atm_nwp_gal9-C896_MG_720t_perfRun0_ex1a_cce_production-32bit works fine with this change
|
Sorry I thought this PR had already been approved
I think we are happy with this change. The reason we initially put in double precision sums was to match the UM where they were occasionally finding single precision sums were losing orthogonality and had to be restarted as double precision. However, I haven't seen this problem in lfric and even when running to a tighter tolerance I haven't managed to detect any problems. I suggest we put this on but make sure we monitor for any unexpected failures (it should be fairly obvious with the solver failing to converge) and if so we can come up with a strategy to address these issues (maybe either defaulting back to double precision or just restarting the sum in double precision) |
6 participants
PR Summary
Sci/Tech Reviewer: thomasmelvin
Code Reviewer: Oakley Brunt (@oakleybrunt)
This PR is a like-for-like replica of the Trac ticket #376. This involves replacing the mixed precision inner product subroutines with PSyclone built-ins. These replacements have been made mostly in the solver in LFRic core but also in the CMA algorithm integration test in LFRic apps. Due to this fundamental change in core, the KGOs for apps have changed in apps, resulting in this PR.
Code Quality Checklist
Testing
trac.log
Test Suite Results - lfric_apps - mixed-precision-apps/run1
Suite Information
Task Information
❌ failed tasks - 166
⌛ waiting tasks - 2
Security Considerations
Performance Impact
AI Assistance and Attribution
Documentation
PSyclone Approval
Sci/Tech Review
(Please alert the code reviewer via a tag when you have approved the SR)
Code Review