fixes for eamxx running from e3sm repo for sycl changes #6636
Conversation
|
|
This is fine as long as we're OK with doing the changes in this repo rather than in the SCREAM repo. I think it's reasonable in this case. |
No. The next upstream merge to SCREAM will bring in the Hxx changes (original PR) and these current ones, consistently updating SCREAM. |
| @@ -277,7 +277,7 @@ apply_iop_forcing(const Real dt) | |||
|
|
|||
| ElementOps elem_ops; | |||
| elem_ops.init(hvcoord); | |||
| const bool use_moisture = (params.moisture == Homme::MoistDry::MOIST); | |||
| const bool use_moisture = params.use_moisture; | |||
There was a problem hiding this comment.
I just remembered: It's better to remove this line entirely. The local variable use_moisture is not actually used. This is already fixed in the downstream repo.
There was a problem hiding this comment.
That did not work. There are calls in that file that use it, dpxx version of this file is very different.
There was a problem hiding this comment.
Ok.
We might actually not want to merge this PR so that our next up- and downstream merges do not hit conflicts. A better course of action might be to (1) do an upstream merge (E3SM -> SCREAM) to bring in the Hxx changes, fix the issues due to these, then (2) do a downstream merge (SCREAM -> E3SM) to fix the EAMxx build in E3SM.
@bartgol @jgfouca or others, do you have an opinion on the best course of action?
I'll approve this PR as is in case merging this PR is decided as best.
There was a problem hiding this comment.
Simple alternative: We can easily work around these issue with some temporary changes in Hxx. Then after the repos are unified, we'd back out the workarounds. That way, only Hxx code would be touched for now.
There was a problem hiding this comment.
i am not sure what you mean by the alternative?
There was a problem hiding this comment.
Yes, I'll submit a PR later today and see if others object to the approach.
There was a problem hiding this comment.
have disabled the EAMxx tests on PRs (disabled all 4 F tests but 2 W tests
Thanks @mahf708. I looked at other PRs and note that really only this test needs to be disabled: SMS_D_Ln5_P4.ne4pg2_oQU480.F2010-SCREAMv1-MPASSI.ghci-oci_gnu, not all the F cases. Up to you; I'm just pointing out disabling all four F tests is not necessary.
There was a problem hiding this comment.
I can't disable them one by one... The F suite has 4 cases:
matrix:
test:
- SMS_D_Ln5_P4.ne4pg2_oQU480.F2010.ghci-oci_gnu
- ERS_Ld5_P4.ne4pg2_oQU480.F2010.ghci-oci_gnu.eam-wcprod_F2010
- SMS_D_Ln5_P4.ne4pg2_oQU480.F2010-SCREAMv1-MPASSI.ghci-oci_gnu
- ERS_Ld5_P4.ne4pg2_oQU480.F2010-SCREAMv1-MPASSI.ghci-oci_gnu.eamxx-prodThe last two are affected. But I can only disable them all at once, or I have to submit a PR. I think it's okay to disable them all at once because the following W tests run whenever F tests run:
matrix:
test:
- SMS_D_Ld1_P8.ne4pg2_oQU480.WCYCL2010NS.ghci-oci_gnu
- ERS_Ld3_P8.ne4pg2_oQU480.WCYCL2010NS.ghci-oci_gnu.allactive-wcprod_1850There was a problem hiding this comment.
(This shows that I should likely split these tests at some point... this is all the early days... fyi, standalone-single-precision passes because we don't run homme with that, see #6637)
There was a problem hiding this comment.
The simple alternative I proposed is here: #6645.
|
I'm closing this PR now that #6645 is in next. |
fixes for eamxx running from e3sm repo for sycl changes
Fixes #6635
[bfb]