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

update icepack, nag cleanup, remove tabs #361

Merged
merged 2 commits into from
Sep 11, 2019
Merged

update icepack, nag cleanup, remove tabs #361

merged 2 commits into from
Sep 11, 2019

Conversation

apcraig
Copy link
Contributor

@apcraig apcraig commented Sep 6, 2019

PR checklist

  • Short (1 sentence) summary of your PR:
    Update icepack, nag cleanup, remove tabs in code
  • Developer(s):
    apcraig
  • Suggest PR reviewers from list in the column to the right.
  • Please copy the PR test results link or provide a summary of testing completed below.
    tested on 4 compiler on izumi
    #c4c82941 at https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks
  • How much do the PR code changes differ from the unmodified code?
    • bit for bit
    • different at roundoff level
    • more substantial
  • Does this PR create or have dependencies on Icepack or any other models?
    • Yes
    • No
  • Does this PR add any new test cases?
    • Yes
    • No
  • Is the documentation being updated? ("Documentation" includes information on the wiki or in the .rst files from doc/source/, which are used to create the online technical docs at https://readthedocs.org/projects/cice-consortium-cice/.)
    • Yes
    • No, does the documentation need to be updated at a later time?
      • Yes
      • No
  • Please provide any additional information or relevant details below:

-Updated Icepack to current trunk version
-Cleaned up some unused variables as reported by nag compiler
-Removed tabs from source code

eclare108213
eclare108213 reviewed Sep 6, 2019
View reviewed changes
Copy link
Contributor

@eclare108213 eclare108213 left a comment

Choose a reason for hiding this comment

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

Thanks @apcraig, this is good. My only concern is the removal of dyrect in the boxslotcyl_data_aice routine of ice_init.F90. That looks like a bug to me, but perhaps this is intentional, to force the cylinder test to be symmetric? What happens when dxrect /= dyrect? @phil-blain @JFLemieux73 please, would you take a look at this?

@phil-blain
Copy link
Member

You are right, I did not test varying dxrect/dyrect last year ; my mistake.
If dxrect != dyrect currently it will change the location of the cylinder and probably it just won't work at all.
Some instances of "dxrect" should be "dyrect", specifically those that involve the y coordinate :

CICE/cicecore/cicedynB/general/ice_init.F90

Line 1885 in 329318b

center_y = p75*dxrect*(ny_global-1)

CICE/cicecore/cicedynB/general/ice_init.F90

Line 1898 in 329318b

(dxrect*real(jglob(j)-1, kind=dbl_kind) >= slot_y(1)) .and. &

CICE/cicecore/cicedynB/general/ice_init.F90

Line 1899 in 329318b

(dxrect*real(jglob(j)-1, kind=dbl_kind) <= slot_y(2))

CICE/cicecore/cicedynB/general/ice_init.F90

Line 1902 in 329318b

(dxrect*real(jglob(j)-1, kind=dbl_kind) - center_y)**c2) <= radius

phil-blain
phil-blain reviewed Sep 6, 2019
View reviewed changes
cicecore/cicedynB/general/ice_forcing_bgc.F90
@@ -77,7 +77,7 @@ subroutine get_forcing_bgc
use ice_domain, only: nblocks, blocks_ice
use ice_arrays_column, only: ocean_bio_all
use ice_calendar, only: yday
use ice_flux, only: sss
! use ice_flux, only: sss
Copy link
Member

Choose a reason for hiding this comment

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

same here

cicecore/cicedynB/dynamics/ice_dyn_evp.F90
@@ -641,7 +641,7 @@ subroutine stress (nx_block, ny_block, &
tensionne, tensionnw, tensionse, tensionsw, & ! tension
shearne, shearnw, shearse, shearsw , & ! shearing
Deltane, Deltanw, Deltase, Deltasw , & ! Delt
puny , & ! puny
! puny , & ! puny
Copy link
Member

Choose a reason for hiding this comment

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

this one was commented instead of removed, was this intentional?

dabail10
dabail10 reviewed Sep 6, 2019
View reviewed changes
cicecore/cicedynB/analysis/ice_history_bgc.F90
@@ -226,20 +226,20 @@ module ice_history_bgc
n_upNO , n_upNH , &
n_bTin , n_bphi , &
n_iDi , n_iki , &
n_bgc_PON , n_bgc_PON_ml , &
n_bgc_PON , &
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume these BGC variables are never used? I had left a bunch in as they were potentially to be used in the future.

dabail10
dabail10 reviewed Sep 6, 2019
View reviewed changes
cicecore/cicedynB/general/ice_forcing.F90

character (char_len_long) :: &
filename ! name of netCDF file
filename ! name of netCDF file

character(len=*), parameter :: subname = '(ocn_data_hadgem)'
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we test the HadGEM forcing and driver? This might be out of date.

dabail10
dabail10 approved these changes Sep 6, 2019
View reviewed changes
Copy link
Contributor

@dabail10 dabail10 left a comment

Choose a reason for hiding this comment

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

Looks good. I made a few general comments.

@apcraig
Copy link
Contributor Author

apcraig commented Sep 6, 2019

Thanks everyone for having a look. The cleanup was somewhat ad-hoc based on some nag compiler messages. I thought about leaving the dyrect in and thought it was odd that it was not used. In some ways, I removed it to highlight that issue, I'm glad folks are having a second look. I'm happy to reintroduce dyrect and/or make other changes, although maybe if some other mods are needed, it's better as a new PR. I commented out some variables because there is commented out source code that might use them. I am happy to re-add any possible future variables, although I think it makes sense to add them back later when they are implemented. Regarding the hadgem forcing, there are always likely to be some options/features that we cannot test, but that the community has contributed or wants/needs. I think it's fine to leave those in and put the burden on the community to maintain. If we know something is not needed or very broken, we can remove it, but we should be conservative when we do. What we really need is the code coverage tool working (I need to get back to that) so we can more clearly track what we're testing and also more readily identify possible dead code that needs to be removed.

If anyone wants a particular change/reversion in this PR, just let me know.

@eclare108213
Copy link
Contributor

I think the dyrect issue should be fixed as @phil-blain notes above. If dyrect=dxrect then hopefully this will not impact our current tests. Otherwise this PR looks good to go.

@apcraig
Copy link
Contributor Author

apcraig commented Sep 9, 2019

Sounds good, I will modify the 4 lines proposed by @phil-blain, run some tests, and then update the PR.

@apcraig
Copy link
Contributor Author

apcraig commented Sep 10, 2019

The dyrect mods have been pushed. I verified everything was bit-for-bit with this change in full test suites on izumi with 4 compilers.

@eclare108213 eclare108213 merged commit d709a16 into CICE-Consortium:master Sep 11, 2019
@apcraig apcraig deleted the nagclean branch August 17, 2022 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phil-blain phil-blain phil-blain left review comments

@eclare108213 eclare108213 eclare108213 left review comments

@dabail10 dabail10 dabail10 approved these changes

Assignees
No one assigned
Labels
Icepack Software Engineering
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@apcraig @phil-blain @eclare108213 @dabail10