-
Notifications
You must be signed in to change notification settings - Fork 54
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
Branch of fesom2.6 including recom and tracer parallelisation #681
base: main
Are you sure you want to change the base?
Conversation
Define new variables to track tracer changes due to advection and diffusion. We want to save for now diffusion and advection contribution to the tracer changes. Horizontal and vertical diffusion includes Redi parametrization (if it is set .true.).
Fill __ciso directive to ensure that carbon isotope code works. Medusa interface is added.
By compilation I found an issue regrading new compilations. I need to manually delete the executable file fesom.x in fesom2.build/bin and then compile the model. Otherwise, after a successful compilation I will still find the old fesom.x in fesom2/bin. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of review:
There were a few places that need changing, some that may not need to change but would require a closer look that everything is indeed ok. There are a number of style improvements to be made. All comment that start with ! kh date
should start without the name and date. Same for !YY
, !O:G
!OG
and !.OG.
Git blame can tell you this info.
In addition to the standalone FESOM2 CI tests, this branch needs to be tested by me as part of AWI-CM3, by @ackerlar as part of AWI-ESM2, by @sebastianbeyer or @suvarchal as part of IFS-FESOM, by @mbutzin with active tracers. I'm assuming here, that you have already tested this branch with recom and without _usetp
CMakeLists.txt
Outdated
@@ -15,7 +15,7 @@ set(OIFS_COUPLED OFF CACHE BOOL "compile fesom coupled to OpenIFS. (Also needs F | |||
set(CRAY OFF CACHE BOOL "compile with cray ftn") | |||
set(USE_ICEPACK OFF CACHE BOOL "compile fesom with the Iceapck modules for sea ice column physics.") | |||
set(OPENMP_REPRODUCIBLE OFF CACHE BOOL "serialize OpenMP loops that are critical for reproducible results") | |||
set(RECOM_COUPLED OFF CACHE BOOL "compile fesom including biogeochemistry, REcoM3") | |||
set(RECOM_COUPLED ON CACHE BOOL "compile fesom including biogeochemistry, REcoM3") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be turned back for the main branch.
@@ -20,6 +20,9 @@ MODULE MOD_TRACER | |||
real(kind=WP) :: tra_adv_pv = 1. ! a parameter to be used in horizontal advection (for QR4C it is the fraction of fourth-order contribution in the solution) | |||
integer :: AB_order=2 | |||
integer :: ID | |||
!___________________________________________________________________________ | |||
! TODO: Make it as a part of namelist.tra |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be done before more?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to clarify with Özgür
if(num_fesom_groups > 1) then | ||
call MPI_Bcast(atm_net_fluxes_north(1), nrecv, MPI_DOUBLE_PRECISION, 0, MPI_COMM_FESOM_SAME_RANK_IN_GROUPS, MPIerr) | ||
call MPI_Bcast(atm_net_fluxes_south(1), nrecv, MPI_DOUBLE_PRECISION, 0, MPI_COMM_FESOM_SAME_RANK_IN_GROUPS, MPIerr) | ||
end if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
beautification: alignment
@@ -252,28 +270,56 @@ subroutine restart(istep, nstart, ntotal, l_read, which_readr, ice, dynamics, tr | |||
logical rawfiles_exist, binfiles_exist | |||
logical, save :: initialized_raw = .false. | |||
logical, save :: initialized_bin = .false. | |||
integer mpierr | |||
|
|||
! integer mpierr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, since only partit%mpierr is used here
if (tracers%data(tr_num)%ID == 14) tracers%data(tr_num)%values(:,:) = tracers%data(tr_num)%values(:,:) * exp(-decay14 * dt) | ||
if (tracers%data(tr_num)%ID == 39) tracers%data(tr_num)%values(:,:) = tracers%data(tr_num)%values(:,:) * exp(-decay39 * dt) | ||
|
||
!YY: C14 seems to be calculated both in fesom and recom |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarify with @mbutzin
@@ -270,10 +389,93 @@ subroutine solve_tracers_ale(ice, dynamics, tracers, partit, mesh) | |||
end if | |||
call exchange_nod(tracers%data(tr_num)%values(:,:), partit) | |||
!$OMP BARRIER | |||
end do | |||
! end do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirm that now needed anymore. I can't see right now where the start of the loop was removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not needed here but moved to L424.
In addition to the changes of the review, the merge conflicts should be solved. Most of them look like simple double additions of new features that both have to be kept. e.g. parallel tracers and icebergs. |
Is there any template fesom-recom yaml that I can use for testing? |
I get this error during compilation:
|
I am still testing the code. So far I can run ocean-only without __usetp in CMakeLists and compile the model with __usetp. But I have not yet tested the coupled setup. |
I just compiled it with FESOM_COUPLED ON and didn't get any error. Did you compile the model with esm-tools? |
@a270105 yes, I used esm_tools: which oasis version are you using? |
I got at least rid of this error
when switching to oasis branch |
call cpl_oasis3mct_init(f%partit,f%partit%MPI_COMM_FESOM) | ||
! call cpl_oasis3mct_init(f%partit,f%partit%MPI_COMM_FESOM) | ||
! kh 02.12.21 | ||
#if defined(__recom) && defined(__usetp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"localCommunicator" is neither defined in fesom_module nor in MOD_PARTIT. Should this really be "localCommunicator" or "f%partit%MPI_COMM_FESOM"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't really answer this. These lines already exited in fesom2.5-recom and another earlier version where Özgür and I merged their complex recom version and my simpler paleo version. But I can't find them in my paleo version where Kai first time implemented tracer parallelisation. There this part of code is still in fvom_main.f90 and only contains:
#if defined (__oasis)
! kh 21.03.22 pass num_fesom_groups to coupler
call cpl_oasis3mct_init(MPI_COMM_FESOM, num_fesom_groups)
#endif
t1 = MPI_Wtime()
call par_init
......
So I think it should be for the case with tracer parallelisation:
call cpl_oasis3mct_init(f%partit, f%partit%MPI_COMM_FESOM, num_fesom_groups)
FESOM compiled for me with some minor changes in MPI calls. I also changed
in However, the model crashes as FESOM is missing some entries in namelist.config. Is there a recom specific namelist.config? |
|
Do you compile with __usetp? If not, there is no additional entry needed in namelist.config. |
Yes, I set
in |
Yes, with that option you would need an additional part in namelst.config to control the tracer groups. Since the version with tracer parallelisation is still being tested, we should try first with the second option:
|
Without
|
@ackerlar: I found some similar issues, when i was playing around on juwels last week with GCC compiler. There were different locations in the code where we tried to associate a pointer to a variable via the associate_part_ass.h and associate_mesh_ass.h file that wasnt allocated yet. The happend especially during the initialisation phase! You can try to use the extended debuggin compiler flags and also the namelist.config flag_debug=.True., to better track down where it happens.
|
I created a new branch of esm-tools and added a new model 'awiesm-2.6-recom'. The branch is called In config/couplings I included |
Now I got these errors by compiling awiesm-2.6-recom as well. flag_debug is set to true and I didn't get more information. I tried with |
…nto fesom2.6_recom_tp update fesom-2.6
Tracer parallelisation can be switched on by __usetp in fesom2/src/CMakeLists.txt and onlye used when recom is on.