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

CICE: Floe size distribution #382

Merged
merged 77 commits into from
Dec 7, 2019
Merged

Conversation

eclare108213
Copy link
Contributor

@eclare108213 eclare108213 commented Nov 27, 2019

PR checklist

  • Short (1 sentence) summary of your PR:
    Changes to CICE driver to implement floe size distribution in Icepack.
  • Developer(s):
    Lettie Roach, Elizabeth Hunke
  • 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.
    Passes basic tests; currently undergoing more extensive testing
  • 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/. A test build of the technical docs will be performed as part of the PR testing.)
    • Yes
    • No, does the documentation need to be updated at a later time?
      • Yes
      • No
  • Please provide any additional information or relevant details below:

This PR is not yet ready to merge, pending

  • move ww3 forcing file from cicecore to ftp
  • add abort if fsd and bgc are both on (like Icepack driver)
  • additions to PIO history and restart modules.
  • execute full test suites
  • run all tests with -fsd12 to see what breaks
  • perform a longer run with fsd12 and ww3 forcing and check the output for plausibility
  • Revert Icepack here to the master version and update it after the Icepack PR is merged
  • Review documentation for completeness and consistency with Icepack's
  • Compare CICE and Icepack driver changes to see if they are consistent and we didn't miss something
  • merge Icepack PR

See also FSD project board

@eclare108213
Copy link
Contributor Author

eclare108213 commented Dec 6, 2019

And the build fails because it has the older Icepack version, which will be fixed when the new PR is merged and updated in CICE.

@apcraig
Copy link
Contributor

apcraig commented Dec 6, 2019

Right, reverting back to the current Icepack version in CICE is not what we wanted. When we committed the ifsd1 PR to the Icepack, I pushed an update to @lettie-roach fsd branch to point to that. We'll need to do that again once we push ifsdech branch to the Consortium. The fact it's failing now is OK, I'll update Icepack again when we push the next PR.

@eclare108213
Copy link
Contributor Author

Okay, I'll try to stay out of the way!

@eclare108213
Copy link
Contributor Author

Except for moving the ww3 file to ftp and final testing, this PR looks ready to merge.

@apcraig
Copy link
Contributor

apcraig commented Dec 7, 2019

I think this is ready to merge. The testing looks good. https://github.com/CICE-Consortium/Test-Results/wiki/cice_by_hash_forks hash a7080a4. I will merge this now.

@apcraig apcraig merged commit 29b99b6 into CICE-Consortium:master Dec 7, 2019
@daveh150
Copy link
Contributor

daveh150 commented Jan 6, 2020 via email

@eclare108213
Copy link
Contributor Author

If you are running with any ice at all, you ought to have at least one floe size category! I would think that nf=1 should be the default when tr_fsd=F, but maybe it's not.

@duvivier
Copy link
Contributor

duvivier commented Jan 6, 2020

Maybe this is a documentation item. We should put in there that nf must be .ge. 1. @duvivier - do this asap!

@daveh150
Copy link
Contributor

daveh150 commented Jan 6, 2020 via email

@eclare108213
Copy link
Contributor Author

Agreed. If the FSD is off, then there is no need to have nf in the history output. However it doesn't take up much space and would make it clear exactly how many floe size categories were in use. Also, if someone's analysis software expects it to be there and breaks because it isn't, they might be a little annoyed (but that just shows lack of flexibility in their software). I think we should make it clear in the doc that nf >= 1, as @duvivier suggests. I'm open to others' opinions about whether nf should be included in the output when it isn't used as a netcdf dimension.

@apcraig
Copy link
Contributor

apcraig commented Jan 6, 2020

I would argue if there is a clean way to turn off fsd (tr_fsd) then the value of nf should not matter and it should be fine to be zero or anything else when fsd is off. And that (tr_fsd) should also control whether the fsd output is added to the history/restart or not. I would argue that this is the most appropriate way to handle all optional features / output fields. Would this be a lot of work? I'm willing to have a look and help clean this up if it's not too complicated. If it's challenging to fix properly, then I am fine with the strategy that nf >=0 for all cases. We should also add a run time check to verify a valid value. It should be more than just documentation. That same strategy should be applied to any other optional tracer that also needs to be >=0 even if it's not turned on.

@eclare108213
Copy link
Contributor Author

It is fine with me for tr_fsd to control whether nf is added to the history/restart files. @apcraig I accept your offer to take a look and clean it up if it's straightforward! Thanks. However I do think that the default value should be nf=1 when FSD is off, regardless of whether it's written to the files, because that is the physically meaningful value. It's possible that some other parameterization will need/want to use it in the future.

phil-blain added a commit to phil-blain/CICE that referenced this pull request Sep 22, 2020
This '+' sign was copy-pasted there in error in 29b99b6 (CICE: Floe size
distribution  (CICE-Consortium#382), 2019-12-07). Remove it.
phil-blain added a commit to phil-blain/CICE that referenced this pull request Dec 17, 2020
This '+' sign was copy-pasted there in error in 29b99b6 (CICE: Floe size
distribution  (CICE-Consortium#382), 2019-12-07). Remove it.
phil-blain added a commit to phil-blain/CICE that referenced this pull request Apr 28, 2021
This '+' sign was copy-pasted there in error in 29b99b6 (CICE: Floe size
distribution  (CICE-Consortium#382), 2019-12-07). Remove it.
phil-blain added a commit to phil-blain/CICE that referenced this pull request Jun 2, 2021
This '+' sign was copy-pasted there in error in 29b99b6 (CICE: Floe size
distribution  (CICE-Consortium#382), 2019-12-07). Remove it.
phil-blain added a commit to phil-blain/CICE that referenced this pull request Jun 2, 2021
This '+' sign was copy-pasted there in error in 29b99b6 (CICE: Floe size
distribution  (CICE-Consortium#382), 2019-12-07). Remove it.
apcraig pushed a commit that referenced this pull request Jun 10, 2021
* drivers/hadgem3: add missing 'subname' and use existing 'subname's

* drivers/hadgem3/CICE_InitMod: update 'init_lvl' call

Add the required 'iblk' argument.

* drivers/hadgem3/CICE_RunMod: remove uneeded 'dt' arguments

The subroutines 'prep_radiation', 'zsal_diags', 'bgc_diags' and 'hbrine_diags'
do not take a 'dt' argument anymore, so remove it.

* drivers/hadgem3/CICE_RunMod: get 'Lsub' from Icepack

* drivers/hadgem3/CICE_RunMod: remove 'da_state_update' subroutine

This subroutine is inside an 'ICE_DA' CPP, which is not used in
any configuration. Remove it.

* drivers/hadgem3/CICE_RunMod: remove stray '+'

This '+' sign was copy-pasted there in error in 29b99b6 (CICE: Floe size
distribution  (#382), 2019-12-07). Remove it.

* drivers/hadgem3: remove obsolete 'check_finished_file' subroutine

Remove the call to 'check_finished_file' as well as the definition
of the subroutine, as the 'hadgem3' driver is not used on machine 'bering'
and it's unclear if machine 'bering' still exists.

* drivers/hadgem3: fix cice_init so it calls 'count_tracers'

This was forgotten back in 8b0ae03 (Refactor tracer initialization (#235), 2018-11-16)

* drivers/hadgem3/CICE_RunMod: add call to 'save_init'

The hadgem3 driver was not updated when 'save_init' was added in 83686a3
(Implement box model test from 2001 JCP paper (#151), 2018-10-22). As
this subroutine is necessary to ensure proper initialization of the
model, add it now.

* drivers/hadgem3/CICE_RunMod: tweak loop indices in 'coupling_prep'

Other drivers use 'ilo,ihi' and 'jlo,jhi' here. Do the same.

* drivers/hagdem3: update driver to new time manager

* drivers/hadgem3: pass 'mpi_comm_opa' explicitely to init_communicate

In 066070e (Fix minor issues in documentation, key_ CPPs, bfbcomp return
codes (#532), 2020-11-23), the 'ice_communicate' module was updated to
remove CPP macros relating to the OASIS coupler (key_oasis*) and to the
NEMO ocean model (key_iomput). These CPPs were used to make the correct
MPI communicator accessible to the 'init_communicate' subroutine.
However, that subroutine already accepts an optional MPI communicator as
argument and it was deemed cleaner to require the driver layer to
explicitely pass the communicator instead of making it accessible
through 'use' statements.

Update the 'hadgem3' driver, used for coupling with NEMO, to explicitely
pass the NEMO MPI communicator 'mpi_comm_opa' to 'init_communicate'.

* drivers: add 'nemo_concepts' driver

Historically the 'hadgem3' driver has been used when compiling a single
NEMO-CICE executable at ECCC.

Going forward, all driver-level adjustements will be done in our own
driver, 'nemo_concepts', 'CONCEPTS' being the name of the
multi-departmental collaboration around using the NEMO ocean model.

Copy CICE_InitMod, CICE_RunMod and CICE_FinalMod from the 'hadgem3'
directory to a new 'nemo_concepts' directory under 'drivers/direct'.

The following commits will clean up this new driver and port over some
in-house adjustments.

* drivers/nemo_concepts: remove unused 'writeout_finished_file' subroutine

This subroutine was only called on machine 'bering', which is not an
ECCC machine and probably does not exist anymore anyway. Remove it.

* drivers/nemo_concepts: call 'scale_fluxes' with 'aice_init'

Since 'merge_fluxes' is called with aice_init, it is more consistent to
also call 'scale_fluxes', in 'coupling_prep' with 'aice_init'
instead of 'aice'.

Copy this in-house change to the new 'nemo_concepts' driver.
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.

5 participants