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.

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

Initialize more arrays below kmt #372

Merged
merged 6 commits into from
Mar 16, 2021

Conversation

mnlevy1981
Copy link
Collaborator

Running call_compute_subroutines with the intel compiler using the following
FFLAGS: -g -O0 -init=snan,arrays crashed until I initialized several fields
below the sea floor.

Leaving this is draft status until @mvertens verifies that it fixes the error she was seeing.

Fixes #371

Running call_compute_subroutines with the intel compiler using the following
FFLAGS: -g -O0 -init=snan,arrays crashed until I initialized several fields
below the sea floor.
@mnlevy1981
Copy link
Collaborator Author

It would be useful to figure out what gfortran flags can be set in order to detect this error, then our continuous integration can ensure similar problems don't slip back in.

@mnlevy1981
Copy link
Collaborator Author

Also, there might be similar problems with the ciso code but we don't have a test for that

Verified that using -finit-real=snan -ffpe-trap=invalid on the previous head of
development causes the call_compute_subroutines test to crash but the code on
the fix_uninit_mem branch (which eliminates unitialized values in the sinking
particle type below the sea floor) runs successfully.
@mnlevy1981 mnlevy1981 marked this pull request as ready for review February 26, 2021 00:29
@mnlevy1981
Copy link
Collaborator Author

Verified this is sufficient to get @mvertens's test to pass

It turns out that we need PON_remin=0, but none of the %remin(k) terms are used
if k > column_kmt. Introduced
marbl_interior_tendency_share_set_used_particle_terms_to_zero() to handle
setting sflux_out(k) and hflux_out(k) to 0. Next commit will add calls to the
same subroutine in ciso (which currently isn't tested for uninitialized memory)
Also added a comment to the else statement to make it clear which "if" it is
tied to
In the last commit I added calls to the new share routine to set hflux_out and
sflux_out below the sea floor for ciso sinking particles but I forgot to add
the additional dzr_loc = c0 line that is already in the non-ciso code and it
was caught by our CI scripts.
@mnlevy1981
Copy link
Collaborator Author

@klindsay28 as it turns out, the -finit-real=snan flag isn't sufficient to trap unitialized real variables in a derived type. I added -finit-derived per some gfortran documentation, except it includes the note

These options do not initialize

  • objects with the POINTER attribute
  • allocatable arrays
  • variables that appear in an EQUIVALENCE statement.

And these are allocatable arrays. So when cheyenne is back up I'll add -init=snan,arrays to the intel flags, verify it's aborting when I don't initialize hflux_out and sflux_out, and then use it to see if I also need to initialize remin (currently marbl_interior_tendency_share_set_used_particle_terms_to_zero() does not set sinking_particle%remin(k) = c0 because I was too trusting of the gfortran flags, but I suspect I'll need to add that line back in). Then I'll test in CESM and, assuming all tests pass, merge this to development.

Testing on cheyenne with intel also showed that we need to intialize remin to 0
below kmt in marbl_interior_tendency_share_set_used_particle_terms_to_zero()
@mnlevy1981
Copy link
Collaborator Author

Per the previous comment, I tried running the stand-alone test with just hflux_out and sflux_out set to 0 below the sea floor, and the intel compiler didn't like that. So dd7cb41 does two things:

  1. sets remin to 0 below the sea floor
  2. updates the compile flags for running the stand-alone driver with intel

I tested that commit on cheyenne by running aux_pop_MARBL with both intel and gnu -- both sets of tests were bit-for-bit identical with baselines, so I'll merge this in and make a new tag.

@mnlevy1981 mnlevy1981 changed the base branch from stable to development March 16, 2021 22:21
@mnlevy1981 mnlevy1981 merged commit 4ded116 into marbl-ecosys:development Mar 16, 2021
@mnlevy1981 mnlevy1981 deleted the fix_uninit_mem branch August 25, 2021 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make sure all arrays are initialized before use
1 participant