-
Notifications
You must be signed in to change notification settings - Fork 139
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
solve_zsal bug fix and update zsal test option #549
Conversation
I don't entirely understand the test results: 268 measured results of 268 total results The failures are the usual 40-proc configurations that always fail on badger and a bunch of decomp tests, which weren't run in the CICE v6.1.4 base_suite. I don't understand why they were run in the new code, since the base_suite script is identical. @apcraig Do you know what is causing the decomp tests to run in my new code? consortium master:
my fork:
Regardless, I think the zsal tests are okay for now, though probably still not bug-free. @apcraig please check whether this fixes the problem on cheyenne noted in #548. Thanks! Should we merge Icepack PR#346 first then update Icepack in CICE as part of this PR? |
Also, travis seems to be hung for both this PR and the icepack one. |
Regarding Travis: indeed I don't see either PR at https://travis-ci.com/github/CICE-Consortium/CICE/pull_requests (CICE) or https://travis-ci.com/github/CICE-Consortium/Icepack/pull_requests (Icepack). I know that Travis changed their plan recently, and free plans now have a maximum "credits", i.e. build minutes. The announcement blog post says the 10K credit for free plans is equivalent to around 1000 minutes, so maybe we have run out... It also says as an open-source project we can apply for more free credits... or we could consider switching to GitHub Actions instead. |
travis-ci is currently disabled for us. See #550. |
I have verified that this change in addition tested with the Icepack change CICE-Consortium/Icepack#346 fixes the zsal failing test (although I think Icepack changes have no role). I am now running a full test suite with both sets of changes. If all goes well, I propose we merge CICE-Consortium/Icepack#346 and as well as this PR. This PR could also be updated to include the newer Icepack as well, or that could come via a separate PR. |
Full test suite results on cheyenne with 3 compilers is here, https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#dd6e38f84da340b7956dd8a10af04627a1725334. Everything looks good. @eclare108213, if you want to change this PR from a draft and update Icepack after merging the Icepack zsal PR there, that would be fine. Alternatively, we can just merge this if it's ready. Let me know how I can help. |
I'll update icepack after CICE-Consortium/Icepack#346 is merged. |
@eclare108213, any chance to update icepack and switch this PR from draft mode? We could also update icepack and merge this now. Either way. |
Sorry, I haven't had a chance to come back to this. @apcraig would you have time to update Icepack, test and merge? I think it's ready. |
I just updated Icepack. I ran full test suites before and verified (via diffs) that this PR is exactly what was tested before. I will also change the draft setting and we should be able to merge after any other reviews and @eclare108213 agrees it's ready. |
I think we should go ahead and merge this. |
@eclare108213
see comments below
The bug fix is simply to set nltrcr=1 when solve_zsal=T.
The z_tracers flag turns on vertical biogeochemistry, which is not needed to test the zsalinity capability.
Testing is underway.
Icepack PR#346 is related but independent of these changes.