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

Transient tracers bugfixed #659

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Transient tracers bugfixed #659

wants to merge 11 commits into from

Conversation

mbutzin
Copy link
Collaborator

@mbutzin mbutzin commented Dec 19, 2024

This branch includes bugfixes, missing and improved code and updated namelists to run FESOM-2.6 with transient tracers (14C, 39Ar, CFC-11, CFC-12, SF6).
Note that without / prior to these changes the model could be compiled without errors but the simulation would fail at runtime.

@mbutzin
Add 'otracers' which are necessary for transient tracer output.
b8075ae
@mbutzin
Include logical variables to swich on/off specific transient tracers.
a671797
@mbutzin
Bugfixes: include missing transit tracer code and control output.
092e333
@mbutzin
Bugfix: amend incorrect transit tracer input.
23819e5
@mbutzin
This version comes from FESOM in AWIESM-2.6 but does not include any …
8884e79 
…changes wrt transient tracers.
@mbutzin
Change output file names of transient tracers from 'tra_ ...' to expl…
78836ca 
…icit tracer names, e.g., 'cfc11'.
@mbutzin
Include logical variables to swich on/off specific transient tracers.…
7620703 
… Include CFC-11. Change from hard-wired I/O unit numbers with variable I/O unit values.
@mbutzin
Boundary conditions of transient tracers are now handled by function …
6db046f 
…'bc_surface', remove the now obsolete function 'transit_bc_surface' and its interface module.
@mbutzin
IDs of transient tracers are no longer hard-wired. Include initializa…
ca08d96 
…tion of CFC-11.
@JanStreffing
Copy link
Collaborator

There seems to be a problem with this branch right now. The tests fail with:

  560 |     use transit_bc_surface_interface
      |         1
Fatal Error: Cannot open module file 'transit_bc_surface_interface.mod' for reading at (1): No such file or directory
compilation terminated.

@mbutzin
Copy link
Collaborator Author

mbutzin commented Dec 19, 2024

There seems to be a problem with this branch right now. The tests fail with:

  560 |     use transit_bc_surface_interface
      |         1
Fatal Error: Cannot open module file 'transit_bc_surface_interface.mod' for reading at (1): No such file or directory
compilation terminated.

I removed L 560 which is obsolete now. Let me know if there is still a problem.

JanStreffing
JanStreffing requested changes Dec 19, 2024
View reviewed changes
Copy link
Collaborator

@JanStreffing JanStreffing left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Checks are all passing now. But there seem to be a few changes that look like they undo recent fixes unrelated to tracers. Some changes / undoing some of these will be needed.

src/gen_modules_clock.F90 Outdated
@@ -2,8 +2,6 @@ module g_clock
!combining RT and Lars version
!
use g_config
use iso_fortran_env, only: error_unit
use mpi
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this and other changes to gen_modules_clock intended?

src/io_meandata.F90
end if
CASE ('h_ice ')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the removal of h_ice and h_snow intended?

src/io_meandata.F90 Outdated
@@ -385,7 +377,7 @@ subroutine ini_mean_io(ice, dynamics, tracers, partit, mesh)
!_______________________________________________________________________________
! output surface forcing
CASE ('fh ')
call def_stream(nod2D, myDim_nod2D, 'fh', 'heat flux', 'W/m2', heat_flux_in(:), io_list(i)%freq, io_list(i)%unit, io_list(i)%precision, partit, mesh)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I seem to recall this was a recent fix, that seems to be made undone here. Probably a mistake.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Strange ... I did not touch gen_modules_clock.F90 and io_meandata.F90 at these lines.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It it probably because you based you changes upon an older version of fesom. I'd suggest you rebase your changes onto the latest main

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When I run
esm_master install-awiesm-2.6
esm_master install-fesom-2.6
esm_master install-fesom-2.6-paleodyn
I always obtain io_meandata.F90 without these lines.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's because we download FESOM 2.6.1 right now via esm_tools. But here we are merging in the main branch. These lines have been additions to the main branch in the last few weeks. The strange thing for me, is that git removes them again in this PR, because you don't touch the same lines in your branch.

We can have a look together after the holidays to sort this out.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the status of this PR? Can I help with anything?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Something went wrong with branching off from the tag and merging back into main. For some reason doing so would remove the changes on main.

I suggest reimplementing in a branch off from main, and merging back into main.

@ackerlar
Copy link
Collaborator

ackerlar commented Jan 10, 2025 •
edited
Loading

I tried to add the changes made to fix the transient tracers, based on the latest main version. @mbutzin could you have a look and try whether everything works for you https://github.com/FESOM/fesom2/tree/fix/tracers_in_main

I also created a PR #662

UPDATE:
I closed the above mentioned PR as the the changes were the same as in this PR here. Instead, I updated this branch and hope to address all required changes. @mbutzin please have a loot at the updated branch to see whether everything still works for you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ackerlar ackerlar ackerlar left review comments

@JanStreffing JanStreffing JanStreffing requested changes

@sebastianbeyer sebastianbeyer sebastianbeyer left review comments

Requested changes must be addressed to merge this pull request.

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

Successfully merging this pull request may close these issues.
4 participants
@mbutzin @JanStreffing @ackerlar @sebastianbeyer