Fix minor issues in documentation, key_ CPPs, bfbcomp return codes by apcraig · Pull Request #532 · CICE-Consortium/CICE

apcraig · 2020-11-21T18:56:18Z

PR checklist

This PR addresses a handful of minor updates defined by the following issues

quickstart documentation points to porting (#529)
check additional return codes in the bfbcomp tool (#524)
fix undefined variable in ice_init output (#520)
add documentation about aliases (#523)
remove key_ CPPS, can be handled by passing communicator thru interface (#498)

quickstart documentation points to porting (CICE-Consortium#529) check additional return codes in the bfbcomp tool (CICE-Consortium#524) fix undefined variable in ice_init output (CICE-Consortium#520) add documentation about aliases (CICE-Consortium#523) remove key_ CPPS, can be handled by passing communicator thru interface (CICE-Consortium#498)

eclare108213

In the new aliases section of the documentation, I think it should be made clear that these aliases are not automatically set up, and the user would need to copy them into one of their own scripts. (Right?)

phil-blain

Thanks for these @apcraig. I left a few comments below, but the rest looks good.

phil-blain · 2020-11-23T17:53:36Z


 Software requirements are noted in this :ref:`software` section.

+Porting information can be found in the :ref:`porting` section and a special porting section for personal computers is in the :ref:`laptop` section.


Porting was already mentioned in the paragraph below the shell example. Maybe we should clean up that paragraph also ? I think linking to the Porting and Laptop sections here makes sense.

I have cleaned up the quick start. For some reason, readthedocs isn't showing the latest, but I have confirmed offline that the file changes show in the PR diff are working in sphinx and I assume they'll show up when the PR is merged.

phil-blain · 2020-11-23T17:54:36Z

-    alias cdcase 'cd `\grep "setenv ICE_CASEDIR" cice.settings | awk "{print "\$"NF}"`'
-
+- When the run scripts are submitted, the current **ice_in**, **cice.settings**, and **env.[machine]** files are copied from the case directory into the run directory.  Users should generally not edit files in the run directory as these are overwritten when following the standard workflow.  **cice.settings** can be sourced to establish the case values in the login shell.
+- Some useful alias can be found in the :ref:`aliases` section


Suggested change

- Some useful alias can be found in the :ref:`aliases` section

- Some useful aliases can be found in the :ref:`aliases` section

phil-blain · 2020-11-23T17:57:57Z

+Useful aliases
+-------------------
+
+This section provides a list of some potentially useful aliases that leverage the CICE 


Suggested change

This section provides a list of some potentially useful aliases that leverage the CICE

This section provides a list of some potentially useful shell aliases that leverage the CICE

phil-blain · 2020-11-23T17:59:05Z

+
+The current **ice_in**, **cice.settings**, and **env.[machine]** files are copied from 
+the case directory into the run directory when the model is run.  Users can create aliases 
+leveraging the variables in these files.  Aliases like the following can be established 


Suggested change

leveraging the variables in these files. Aliases like the following can be established

leveraging the variables in these files. Aliases like the following can be established by copying them to one's shell startup files

Everyone has their own style to usage in their home shell. I prefer to avoid telling people how to work. My thought here is that we should (at most) demonstrate some possible aliases without encouraging them or micro managing how people work.

I understand that, but some users (I'm thinking students) might not know at all how they should use these aliases. By mentioning "shell startup files", it at least gives them something they can Google to learn how to do it. More advanced users that want to work differently can just disregard that sentence and do as they please.

I'm updated this section a bit and I think it's clearer. Thanks for the feedback.

phil-blain · 2020-11-23T18:03:45Z

+  alias cice_rundir='cice_var ICE_RUNDIR'
+  # open a tcsh shell and source env.* and cice.settings (useful for launching CICE in a debugger)
+  alias cice_shell='tcsh -c "cice_env; tcsh"'
+


The cdcase, cdrun and cice_rundir aliases as written above need the following shell function:

Suggested change

# Print the value of a CICE variable ($1) from cice.settings

cice_var() {

\grep "setenv $1" cice.settings | awk "{print "\$"3}"

}

I didn't notice that. I just cut and paste some of the aliases. Personally, I think these aliases are a bit complicated for many users. But I will try to fix this.

Well, users do not have to understand how the aliases work to benefit from them :)

I added the cice_var alias to the documentation.

phil-blain · 2020-11-23T18:12:17Z

-#if defined key_oasis3 || key_oasis3mct
-   use cpl_oasis3
-#endif
-
-#if defined key_oasis4
-   use cpl_oasis4
-#endif
-
-#if defined key_iomput
-   use lib_mpp, only:   mpi_comm_opa      ! MPP library
-#endif
-


OK, so basically these use statements would have to be in the drivers instead, and the communicator passed to init_communicate by argument. I don't know why it was not made that way in the first place, but I think this is a good change that cleans up the code.

Thanks. I'll update my code next time I rebase the branch where I'm working on coupling with NEMO.

That's correct. I'm actually surprised anyone is using the key_ cpps. I thought that was obsolete by now. If not, we probably need to make sure we call this out in our release notes for Oasis users.

Yes, it definitely should be in the release notes. Also, key_iomput is not OASIS-related, it's plain NEMO. We use it at CMC although we do not use OASIS.

Thanks @phil-blain, very interesting. Do you agree it's reasonable and appropriate to remove these cpps and force users to pass the comm thru the interface?

Yes I think it is.

apcraig · 2020-11-24T00:52:52Z

@eclare108213, I think the items mentioned have been addressed if you want to take another review. Happy to make additional changes.

In 066070e (Fix minor issues in documentation, key_ CPPs, bfbcomp return codes (CICE-Consortium#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/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.

apcraig added Documentation Scripts Drivers/coupling labels Nov 21, 2020

apcraig requested review from dabail10, eclare108213 and phil-blain November 21, 2020 18:56

eclare108213 reviewed Nov 21, 2020

View reviewed changes

Comment thread doc/source/user_guide/ug_running.rst Outdated

update alias documentation

eee1a2d

phil-blain reviewed Nov 23, 2020

View reviewed changes

apcraig added 3 commits November 23, 2020 11:52

update aliases documentation

f60f18b

update alias documentation

67c52ed

work on getting laptops reference working in docs

29ef161

phil-blain approved these changes Nov 23, 2020

View reviewed changes

eclare108213 approved these changes Nov 24, 2020

View reviewed changes

apcraig merged commit 066070e into CICE-Consortium:master Nov 24, 2020

This was referenced Nov 24, 2020

Improve doc for running on personal computers #529

Closed

'decomp' test does not thoroughly check the return code of 'bfbcomp.csh' #524

Closed

for grid_type=tripole, ice_init prints garbage for ns_boundary_type #520

Closed

Update/Remove key_* CPPs #498

Closed

apcraig deleted the docB branch August 17, 2022 21:00


		Software requirements are noted in this :ref:`software` section.

		Porting information can be found in the :ref:`porting` section and a special porting section for personal computers is in the :ref:`laptop` section.

	- Some useful alias can be found in the :ref:`aliases` section
	- Some useful aliases can be found in the :ref:`aliases` section

	This section provides a list of some potentially useful aliases that leverage the CICE
	This section provides a list of some potentially useful shell aliases that leverage the CICE

	leveraging the variables in these files. Aliases like the following can be established
	leveraging the variables in these files. Aliases like the following can be established by copying them to one's shell startup files

+# Print the value of a CICE variable ($1) from cice.settings
+cice_var() {
+\grep "setenv $1" cice.settings | awk "{print "\$"3}"
+}

Conversation

apcraig commented Nov 21, 2020

PR checklist

Uh oh!

eclare108213 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

phil-blain left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apcraig commented Nov 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

phil-blain left a comment •

edited

Loading