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

Cleanup initialization #240

Merged
merged 4 commits into from
Nov 17, 2018
Merged

Conversation

eclare108213
Copy link
Contributor

Cleaned up refactored initialization code somewhat. Removed sil_data_type and nit_data_type, replaced with bgc_data_type.

  • Developer(s): E. Hunke

  • Please suggest code Pull Request reviewers in the column at right.

  • Are the code changes bit for bit, different at roundoff level, or more substantial? BFB in base_suite

  • Please include the link to test results or paste the summary block from the bottom of the testing output below.

https://github.com/CICE-Consortium/Test-Results/wiki/e04a72d86d.pinto.intel.181117.073607

3 tests fail, which are also failing in the master branch:
[eclare@pi-fe1 testsuite.base04]$ results.csh | grep FAIL
FAIL pinto_intel_smoke_gbox128_2x2_boxadv_debug_short run -1 -1 -1
FAIL pinto_intel_smoke_gbox128_2x2_boxadv_debug_short test
FAIL pinto_intel_smoke_gbox128_2x2_boxadv_debug_short compare master02 -1 -1 -1 different-data

This appears to be an array out-of-bounds error in eap:
forrtl: severe (408): fort: (3): Subscript #1 of the array S11R has value -19 which is l
ess than the lower bound of 1
cice 00000000007D0879 ice_dyn_eap_mp_up 1728 ice_dyn_eap.F90
What's causing this?

  • Does this PR create or have dependencies on Icepack or any other models? no

  • Is the documentation being updated with this PR? (Y/N) yes
    If not, does the documentation need to be updated separately at a later time? (Y/N) no

Documentation changes are not tested.

Note: "Documentation" includes information on the wiki and .rst files in doc/source/,
which are used to create the online technical docs at https://readthedocs.org/projects/cice-consortium-cice/.

  • Other Relevant Details:

The changes to bgc_data_type, sil_data_type and nit_data_type make the code slightly less flexible by no longer allowing mixing of silicate and nitrate data sets, or of temperature and salinity data sets. fe_data_type was left in the code because it requires a separate data file, which we do not have in the data repository and do not test. This PR addresses the final remarks in #123.

@apcraig
Copy link
Contributor

apcraig commented Nov 17, 2018

Thanks for removing a file I unintentionally added and for adding the MISS description.

Also, I am seeing the same error in the box debug configuration. I have done some debugging and decided to punt. Looking at the code, it seems surprising that we don't underflow that array all the time. The overflow is kx in

stemp11r = s11r(kx,ky,ka)

where

        nx_yield            =  41, &
!echmod to ensure the angle lies between pi/4 and 9 pi/4
         if (x < piq) x = x + pi2
         dx   = pi/real(nx_yield-1,kind=dbl_kind)   = ~ 0.0785
         invdx = c1/dx  = ~ 12.73
         kx = int((x-piq-pi)*invdx) + 1

So x is between pi/4 and 9pi/4 and kx = (x - 5pi/4)*12.73 + 1. Anytime x > 5pi/4, it seems kx would go more or less negative. To get kx=-19, x would have to be 2.35 or 3pi/4 which, if the normal range of x is pi/4 to 9pi/4, seems ok. Again, don't know what's happening here or why we are seeing this error just in the box case. I think we should try to fix it, but I wouldn't hold up the release because of it. We do not hit that error in non-box configurations. I'm also still waiting for some final commits to the box cases to see if that has an impact.

Should I merge this morning or do you want to wait for additional feedback?

@eclare108213
Copy link
Contributor Author

I recommend to go ahead and merge this, create an issue about the failing box case (if you haven't already) and if we don't figure it out before the release, comment it out of the suite(s) pending further work. I think there's another test that's currently commented out, awaiting additional effort.

@eclare108213
Copy link
Contributor Author

It doesn't have to be done prior to merging, but it would be helpful if @dabail10 could run the refactored code (ice_init.F90, ice_init_column.F90) through his unused variables compiler check. I'm sure I didn't find them all.

@dabail10
Copy link
Contributor

Compiling with NAG now. One issue right away is that there is a call to "flush" in ice_read_write.F90 that should be icepack_warnings_flush.

@dabail10
Copy link
Contributor

Some more:

There is one here:
/home/dbailey/CICE_elizabeth/icepack/columnphysics/icepack_itd.F90, line 671: Variable VICEN_INIT set but never referenced

There are a bunch in ice_init_column.F90. Do you want me to send the output from the compiler?

@apcraig
Copy link
Contributor

apcraig commented Nov 17, 2018

@dabail10 , would you be willing to create a new PR with the variables removed? Alternatively, if you email me the list, I'll do it. We'll probably make it a separate PR from this. thanks!

@apcraig apcraig merged commit 2946264 into CICE-Consortium:master Nov 17, 2018
@dabail10
Copy link
Contributor

I can do this. Most of them are BGC related, so I don't know if Elizabeth wants to leave these in.

@apcraig
Copy link
Contributor

apcraig commented Nov 17, 2018

OK, maybe email us the list then and we'll have a look. thanks!

@eclare108213 eclare108213 deleted the cleanup1 branch August 5, 2019 17:49
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.

3 participants