-
Notifications
You must be signed in to change notification settings - Fork 232
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
New TC variants #1004
New TC variants #1004
Conversation
adcroft
commented
Sep 20, 2019
- Added second variants of tc1
- Added variant of tc2
- Changed shape of tc3 to be 10x8 to be the same size as tc1 and tc2. This will allow us to re-use static memory executables.
- Also changed tense of labels to match directories
- Turned tides off in tc2.a - Uses different remapping scheme, sigma coordinate and different topography.
Codecov Report
@@ Coverage Diff @@
## dev/gfdl #1004 +/- ##
===========================================
+ Coverage 40.42% 43.7% +3.28%
===========================================
Files 212 213 +1
Lines 62242 62355 +113
===========================================
+ Hits 25161 27252 +2091
+ Misses 37081 35103 -1978
Continue to review full report at Codecov.
|
This is where the time is/will be spent with this PR. tc2 is long (11s per test on gaea, 15s on Tavis-CI) and adding a variant adds 2-3 minutes to total testing time. To get full coverage will be expensive so we need to figure out how to exercise code in fewer steps. I'm think non-zero initialization with large numbers might do it?
|
Looking at tc1 vs t2, seems like just the diagnostic mediator alone is 4 seconds longer (6.8 vs 2.8).The rest seems to be 2 sec more in traver advection. I guess ALE is just doing more work here? Also the diagnostics may be more expensive because of layer remapping? |
It's also that we turned on all the parameterizations, tides, etc. tc1 is quite simple by comparison. |
Performance aside, this all worked for me, and it also did not create any new diagnostic errors, so should be OK to merge. This does not affect the code or answers so does not require a formal regression test on Gaea. |
Sorry, I just clicked on some "Ready for Review" feature which was probably meant for you @adcroft. Is this "ready for review" and merge? |
@marshallward I had used the "Draft pull request" option because this was exploratory. Given how much more time is taken for so little coverage gained I think we need to have a hard think about whether the ".a", ".b" derivatives are worth it. There's a "_tc4" waiting to be added too and it only checks an additional hand full of lines. It would be better to replace tc3 with _tc4 and convert tc2.a and tc2.b into full TCs with as many options changed as possible. Let's talk ... |
Sounds like despite the possible inefficiencies, that we should keep marching towards better coverage and so should go ahead and merge this. |
This passed on Travis but the Edit: |