Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Obc tracer fix #507

Merged
merged 12 commits into from
Nov 8, 2023
Merged

Obc tracer fix #507

merged 12 commits into from
Nov 8, 2023

Conversation

kshedstrom
Copy link

Try again at fixing #506. Everything matched at a halo point two in except for h at the very top. Just above this in the code, there is a halo update on h, but just with halo=1.

@codecov
Copy link

codecov bot commented Oct 23, 2023 •
edited
Loading

Codecov Report

Merging #507 (83ed300) into dev/gfdl (feaeb11) will decrease coverage by 0.01%.
The diff coverage is 37.03%.

❗ Current head 83ed300 differs from pull request most recent head e5b194d. Consider uploading reports for the commit e5b194d to get more accurate results

@@             Coverage Diff              @@
##           dev/gfdl     #507      +/-   ##
============================================
- Coverage     37.45%   37.45%   -0.01%     
============================================
  Files           270      270              
  Lines         79675    79692      +17     
  Branches      14818    14823       +5     
============================================
+ Hits          29846    29849       +3     
- Misses        44299    44308       +9     
- Partials       5530     5535       +5
Files Coverage Δ
src/framework/MOM_restart.F90 32.28% <ø> (ø)
src/parameterizations/lateral/MOM_hor_visc.F90 45.36% <100.00%> (ø)
src/core/MOM.F90 50.64% <50.00%> (-0.04%) ⬇️
src/core/MOM_open_boundary.F90 24.21% <26.31%> (-0.03%) ⬇️

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

Hallberg-NOAA
Hallberg-NOAA reviewed Oct 24, 2023
View reviewed changes
Hallberg-NOAA
Hallberg-NOAA requested changes Oct 24, 2023
View reviewed changes
Copy link
Member

@Hallberg-NOAA Hallberg-NOAA left a comment

Choose a reason for hiding this comment

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

Please modify the altered code in MOM.F90 so that the indenting at about lines 1630-1632 matches the structure of the logical block, and that the structure of the logical block at 1629 matches with what is intended.

@kshedstrom
Copy link
Author

Thanks, Bob, sorry about that.

Hallberg-NOAA
Hallberg-NOAA approved these changes Oct 27, 2023
View reviewed changes
Copy link
Member

@Hallberg-NOAA Hallberg-NOAA left a comment

Choose a reason for hiding this comment

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

These changes now look good to me.

@kshedstrom
Copy link
Author

Maybe @marshallward knows why it is now failing the tests?

@marshallward
Copy link
Member

The error:

At line 1663 of file ../../../src/framework/MOM_checksums.F90
Fortran runtime error: Index '4' of dimension 1 of array 'array' outside of expected range (0:0)

The line that is failing:

       call uvchksum("radiation_open_bdry_conds: OBC%tres_[xy]", OBC%tres_x(:,:,:,m), OBC%tres_y(:,:,:,m), G%HI, &
                  haloshift=0, symmetric=sym, scale=1.0)

It doesn't like how the fourth dimension is being passed here. Going through the interfaces may be confusing things, since 4d versions exist and we hope to recasting this thing from 4d to 3d by explicitly setting m.

Or are we actually reshaping it? Is it passing an array of size (:,:,:) or (:,:,:,1)? That may be the confusion. It may need a reshape or a copy before passed to uvchksum.

I don't know how an index range of 0:0 came about here. It could be an internal range set by the compiler to preserve the 4d shape, which we were not meant to see.

I feel like I've done this sort of reduction from 4d to 3d dozens of time without problems, so maybe my words are a distraction from the true issue. I'll have a look and see if I can find anything.

In the meantime, try a reshape or copy and see what happens?

Backtrace for reference:

At line 1663 of file ../../../src/framework/MOM_checksums.F90
Fortran runtime error: Index '4' of dimension 1 of array 'array' outside of expected range (0:0)

Error termination. Backtrace:
#0  0x7f3f1ac23970 in ???
#1  0x7f3f1ac244f9 in ???
#2  0x7f3f1ac24af6 in ???
#3  0x55f5352df730 in __mom_checksums_MOD_chksum_u_3d
	at ../../../src/framework/MOM_checksums.F90:1663
#4  0x55f5352f5bb1 in __mom_checksums_MOD_chksum_uv_3d
	at ../../../src/framework/MOM_checksums.F90:893
#5  0x55f535ff2384 in __mom_open_boundary_MOD_radiation_open_bdry_conds
	at ../../../src/core/MOM_open_boundary.F90:3323
#6  0x55f5356eb708 in __mom_dynamics_split_rk2_MOD_step_mom_dyn_split_rk2
	at ../../../src/core/MOM_dynamics_split_RK2.F90:761
#7  0x55f534def79e in step_mom_dynamics
	at ../../../src/core/MOM.F90:1222
make: *** [Makefile:600: work/tc3/symmetric/ocean.stats] Error 1
#8  0x55f534dfa902 in __mom_MOD_step_mom
	at ../../../src/core/MOM.F90:899
#9  0x55f53567cc73 in mom6
	at ../../../config_src/drivers/solo_driver/MOM_driver.F90:485
#10  0x55f53567e30a in main
	at ../../../config_src/drivers/solo_driver/MOM_driver.F90:27

@kshedstrom
Copy link
Author

Thanks, Marshall. Because it runs for me, it didn't occur to me that this wasn't just the auto-testing being problematic for no good reason.

@marshallward
Copy link
Member

@kshedstrom Just checking, this is ready to go in?

@kshedstrom
Copy link
Author

Yes, I think it's fine.

@marshallward
Copy link
Member

Still getting the whalo error... I will have a look.

@kshedstrom
Copy link
Author

Ooops, sorry, will fix it.

@kshedstrom
Copy link
Author

I don't understand this. When I run the tc tests, the only one to fail is tc3.openmp:

FATAL: fms_affinity_set: OCEAN cpu_set size > allocated storage

@marshallward
Copy link
Member

It seems that the fix for one problem breaks the other, and vice versa.

Hallberg-NOAA
Hallberg-NOAA approved these changes Nov 8, 2023
View reviewed changes
Copy link
Member

@Hallberg-NOAA Hallberg-NOAA left a comment

Choose a reason for hiding this comment

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

This is now functionally correct, and I think that it should be merged in. (I can see how to make the code a bit more succinct at one point with the setting and logging of parameters, but I do not want to hold this up any further over that essentially cosmetic point, now that this is doing the right thing.)

@kshedstrom @Hallberg-NOAA
+Fix for issue NOAA-GFDL#506, tracer OBC bug
16836ba 
 - it only happens in the advection for certain flow directions,
   inflow from OBC plus along-boundary flow.
@kshedstrom @Hallberg-NOAA
Tracer OBCs need more of an h halo update.
5f0faad 
- This one should finally fix the processor count issues with OBCs.
@kshedstrom @Hallberg-NOAA
Correct the "if" statement.
919ace8
@kshedstrom @Hallberg-NOAA
+Adding more halo points to an exchange
5d02408 
 - This will change answers if you start with a non-zero velocity.
   You need three halo points (or maybe cont_stencil) for the
   continuity solver.
 - Also trying to put in some initial DEBUG_OBC code.
@kshedstrom @Hallberg-NOAA
Fix some DEBUG_OBC logic
cbce0b0
@kshedstrom @Hallberg-NOAA
Writing to temporary arrays for tres_xy
adacdb5 
 - Way to trick some compiler.
@kshedstrom @Hallberg-NOAA
Another fix for DEBUG_OBC
1c7a05e
@kshedstrom @Hallberg-NOAA
Fixing the whalo troubles for grids that are 2 wide/long.
f30a2b7
@kshedstrom @Hallberg-NOAA
Exchange all the h_new points.
ff1dd53 
 - without this, because we're remapping all the tres points, it
 dies in debug mode on bad h_new values.
@kshedstrom @Hallberg-NOAA
Trying a different exchange
870aca5 
 - as it was, it was passing my tests, failing the auto-tests.
@kshedstrom @Hallberg-NOAA
Fixing the DEBUG_OBC logging
fb69434
@kshedstrom @Hallberg-NOAA
Putting the logging statement back.
e5b194d 
 - Making an error more verbose too.
@Hallberg-NOAA
Copy link
Member

I am inclined to do a squash-merge on this PR, given the number of small commits and corrections that this PR contains. Would you concur with that approach, @kshedstrom and @marshallward?

@marshallward
Copy link
Member

Fine with me (assumuing it passes regression testing, of course)

@kshedstrom
Copy link
Author

Yes, OK to squash.

@Hallberg-NOAA
Copy link
Member

This PR has passed pipeline testing at gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/21264.

@Hallberg-NOAA Hallberg-NOAA merged commit 0f2a69d into NOAA-GFDL:dev/gfdl Nov 8, 2023
@kshedstrom kshedstrom mentioned this pull request Nov 8, 2023
Closed
@kshedstrom kshedstrom deleted the obc_tracer_fix branch November 10, 2023 23:44
This was referenced Nov 13, 2023
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hallberg-NOAA Hallberg-NOAA Hallberg-NOAA approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kshedstrom @marshallward @Hallberg-NOAA