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

Fix RS/RL type in pkg/steep_icecavity #838

Merged
merged 4 commits into from
Jun 3, 2024
Merged

Conversation

jm-c
Copy link
Member

@jm-c jm-c commented May 25, 2024

What changes does this PR introduce?

Fix few issues in recently added (PR #789) package steep_icecavity

What is the current behaviour?

  1. variables sticfFreshWaterFlux and sticfHeatFlux are declared "_RL" but diagnostics 'SHIICFfw' and 'SHIICFht' are filled by calling DIAGNOSTICS_FILL_RS (i.e., wrong type).
    This only matter when "_RS" is "real*4" and fail to compile with strict S/R argument check, e.g, intel compiler & -devel:
    http://mitgcm.org/testing/results/2024_05/tr_engaging-ifcMpi_20240525_0/summary.txt
  2. missing "Nr" argument when calling S/R ADEXCH_3D_RL in addummy_in_stepping.F prevents to compile AD exp. isomip, as seen here:
    https://mitgcm.org/testing/results/2024_05/tr_svante-ifcAdm_20240526_0/summary.txt
  3. few local arrays in stic_thermodynamics.F are only initialised when useDiagnostics=T but are used even when useDiagnostics=F; this produce a floating point error when checking for un-initialized vars with open64 compiler and "-devel", as here:
    https://mitgcm.org/testing/results/2024_05/tr_engaging-o64Adm_20240526_0/summary.txt

What is the new behaviour

Fix all 3.

Does this PR introduce a breaking change?

no

Other information:

Suggested addition to tag-index

not needed if meged just after PR #789

@jm-c jm-c added the adjoint Affects the adjoint model; label triggers full OpenAD test label May 25, 2024
@jm-c jm-c requested a review from owang01 May 25, 2024 16:59
@jm-c
Copy link
Member Author

jm-c commented May 27, 2024

@owang01 If you could approve this (small) PR quickly I would merge it soon after and I would get some of the daily testreport fixed. Thnaks.

@jm-c
Copy link
Member Author

jm-c commented May 27, 2024

@owang01 Please ignore my previous comment: I saw that there is also some issues that prevent to compile the isomip adjoint (also with ifort & -devel, even without RS=real*4) so I will try to figure out where the problem is.

@jm-c
Copy link
Member Author

jm-c commented May 29, 2024

I think I fixed the problems that are making some daily testreport to fail. I also updated this PR description.
@owang01 If you can review this PR, assuming it's OK, I will merge it soon after to get back all the testreport tests.

@owang01
Copy link
Collaborator

owang01 commented Jun 3, 2024

@jm-c Thank you for the extra fixes. The fixes to DIAGNOSTICS_FILL_RS and missing argument NR in calling S/R ADEXCH_3D_RL look good to me (PR descriptions 1 and 2). For the issue related to the local arrays for diagnostic output in pkg/stic_thermodynamics.F (PR description 3), the assignment of these arrays, such as tmpDiagIcfForcingT and tmpDiagIcfForcingS, also needs to be enclosed by the ifdef block of the CPP option ALLOW_DIAGNOSTICS. I am going to push a new commit to reflect this. Additionally, I will enclose it with the runtime check useDiagnostics. For consistency, I will reinstate the useDiagnostics runtime check when initializing these local arrays, which you removed in commit d32381a.

@jm-c
Copy link
Member Author

jm-c commented Jun 3, 2024

@owang01 Thanks for the review and the improvement. I am going to merge this PR very soon.

@jm-c jm-c merged commit 54f5c8e into MITgcm:master Jun 3, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adjoint Affects the adjoint model; label triggers full OpenAD test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants