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

Update wavespec convergence algorithm #305

Merged
merged 5 commits into from
Mar 12, 2020

Conversation

apcraig
Copy link
Contributor

@apcraig apcraig commented Mar 11, 2020

PR checklist

  • Short (1 sentence) summary of your PR:
    Update wavespec convergence algorithm and other minor cleanups
  • 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 full suites on conrad with 4 compilers, all tests pass and are bit-for-bit except fsd12ww3. Some previously failing tests due to seg faults are now working. I tested both icepack and cice with this version of icepack and results are here Icepack and CICE
    The fsd12ww3 case changes answers when removing allfraclengths due to what seems to be compiler optimization.
  • How much do the PR code changes differ from the unmodified code?
    • bit for bit except fsd12ww3
    • different at roundoff level
    • more substantial
  • Does this PR create or have dependencies on CICE 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 the wavespec convergence algorithm. Removed the large allfraclengths array. This seemed to be leading to segfaults on some machine and some compilers. There was also a bug in that algorithm that did not compute the convergence criteria properly for wave_spec_type = random. The updated algorithm is not bit-for-bit due to compiler optimization but it should produce the same results. It leverages frac_local to accumulate the convergence diagnostic over multiple iterations rather than recomputing from scratch at each iteration. I tested with both constant and random settings and am running a full test suite on conrad with 4 compilers. I also refactored the iterative loop into a while statement.

I added an optional argument to icepack_init_fsd_bounds called write_diags to allow the caller to specify if/when output is written. It should only be written on the root pe. CICE will be leveraging this new argument in a future update to clean up the log file.

Fixed some indentation in the source code.

Update documentation; added a missing namelist variables in the documentation, added the fsd public interfaces to the interface documentation, updated the interface documentation tool to add a new file layer in the interface list, and added a section about columnphysics coding strategies.

…s array, minor cleanup of a few other things
@apcraig apcraig self-assigned this Mar 11, 2020
@apcraig
Copy link
Contributor Author

apcraig commented Mar 11, 2020

This should not be merged until after additional testing is complete. I have created the PR to encourage some early reviews.

@apcraig
Copy link
Contributor Author

apcraig commented Mar 11, 2020

I have run full test suites on conrad and have updated the PR template with the results. Both Icepack and CICE (with this version of Icepack) are working properly, all results are bit-for-bit except the fsd12ww3 case. That case is not bit-for-bit due to compiler optimization with the change to the allfraclengths array. I verified that separately. The results are

https://github.com/CICE-Consortium/Test-Results/wiki/icepack_by_hash_forks#fcde3831d47645335c615a30da4393639cf1e3fe

https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks#7e2a1d99974d21c28a0983721a2453fbf14adbfe

I think this can be merged if others agree. Once merged, I will create a CICE PR that incorporates this version and also fixes the wave_spec_file issue in CICE #415

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.

Looks fine to me. Thanks for sorting it out!

@apcraig
Copy link
Contributor Author

apcraig commented Mar 12, 2020

I just updated the documentation as well as the interface documentation tool. I will run a few more basic tests then I will merge this PR. In particular, the fsd public interfaces did not have the autodocument notation in them. The other thing I did is bring forward the documentation I added in iso1. This might create a conflict in the iso1 PR merge, but we'll fix that when needed. I think it's good to get that documentation update into the next minor release. Finally, I added a new level in the interface documentation associated with file. So instead of just a list of public interfaces, they are organized by file now more clearly. I will update the PR template as well to note these changes.

@apcraig apcraig merged commit edb8c34 into CICE-Consortium:master Mar 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants