diff --git a/.github/workflows/publish_wps.yml b/.github/workflows/publish_wps.yml index 588f4890..588fe599 100644 --- a/.github/workflows/publish_wps.yml +++ b/.github/workflows/publish_wps.yml @@ -4,8 +4,9 @@ on: push: branches: - main + - github_wps pull_request: - types: [opened,reopened,review_requested] + types: [opened, reopened, synchronize] workflow_dispatch: permissions: @@ -20,18 +21,23 @@ jobs: run: | git config --global user.email "umsysteam@metoffice.gov.uk" git config --global user.name "SSD Developers" + - name: Check out source uses: actions/checkout@v4 - - uses: actions/setup-python@v5 + + - name: Setup uv + uses: astral-sh/setup-uv@v6 with: - python-version: '3.x' + python-version: '3.12' + - name: Install Dependencies - run: | - pip install -r .github/workflows/requirements.txt + run: uv sync - - name: build docs - run: | - make clean html + - name: Lint Docs + run: uv run sphinx-lint source + + - name: Build Docs + run: uv run make clean html - name: commit docs to gh-pages branch if: ${{ github.event_name == 'push' && github.ref_name == 'main' }} diff --git a/.github/workflows/requirements.txt b/.github/workflows/requirements.txt index 521c5ba6..bf1eedbf 100644 --- a/.github/workflows/requirements.txt +++ b/.github/workflows/requirements.txt @@ -1,4 +1,8 @@ -Sphinx==6.2.1 -sphinx-design==0.5.0 -pydata-sphinx-theme -sphinx-sitemap +sphinx==8.2.3 +pydata-sphinx-theme==0.16.1 +sphinx-design==0.6.1 +sphinx-copybutton==0.5.2 +sphinx-lint==1.0.0 +sphinx-sitemap==2.8.0 +sphinxcontrib-svg2pdfconverter==1.3.0 + diff --git a/.gitignore b/.gitignore index e79e0976..7c54fe3e 100644 --- a/.gitignore +++ b/.gitignore @@ -1,7 +1,8 @@ -#build output -/build/ - -#IDE files -/.idea -/.venv/ -/venv/ +*.egg-info/ +*.py[cod] +.idea +.venv/ +__pycache__/ +build/ +uv.lock +venv/ diff --git a/README.md b/README.md index 78885907..918abae2 100644 --- a/README.md +++ b/README.md @@ -1,19 +1,61 @@ # Simulation Systems -This repository contains documentation that is common across the many simulation and modelling codes owned by the Met Office. +This repository contains documentation that is common across the many +[simulation and modelling repositories](https://github.com/MetOffice/simulation-systems/wiki) +owned by the Met Office. -To build this documentation from the top level of the project run: +## Build the documentation + +A quick and clean way to get the package dependencies is via +[uv](https://docs.astral.sh/uv/) package manager. + +Ps: Optional system dependencies for PDF generation may require LaTeX +distributions and other third-party libraries. + +### using uv + +```shell +git clone https://github.com/MetOffice/simulation-systems +cd simulation-systems + +# Install dependencies (see pyproject.toml) in project .venv +uv sync +uv run make clean html + +# Verify documentation +firefox build/html/index.html +``` + +### using pip + +Alternatively, if your have Python-3.11 or higher installed (sphinx==8.2.3 +requirement), you can install the dependencies in a virtual environment via pip, +and build the documentation like: + +```shell +cd simulation-systems + + -m venv .venv +source .venv/bin/activate +pip install . +make clean html ``` + +### using conda + +```shell conda env create -f env.yml conda activate sphinx_doc_env make clean html firefox build/html/index.html ``` -The documentation is written in sphinx markup. To develop changes to this +## Contributing + +The documentation is written in sphinx markup. To develop changes to this documentation first create an issue detailing the changes that are required. Then create a branch in a clone of this repository, linking it to your issue and -regularly building your changes as described above. +regularly building your changes as described above. -Once happy with your development create a pull request and request a review -from MetOffice/ssdteam. \ No newline at end of file +Once happy with your development create a pull request and request a review from +[MetOffice/ssdteam](https://github.com/orgs/MetOffice/teams/ssdteam). diff --git a/pyproject.toml b/pyproject.toml new file mode 100644 index 00000000..ca686c8c --- /dev/null +++ b/pyproject.toml @@ -0,0 +1,15 @@ +[project] +name = "simulation-systems" +version = "0.1.0" +description = "Simulation Systems Working Practices" +readme = "README.md" +requires-python = ">=3.11" +dependencies = [ + "pydata-sphinx-theme==0.16.1", + "sphinx==8.2.3", + "sphinx-design==0.6.1", + "sphinx-copybutton==0.5.2", + "sphinx-lint==1.0.0", + "sphinx-sitemap==2.8.0", + "sphinxcontrib-svg2pdfconverter==1.3.0", +] diff --git a/source/Development/Diagnostics/lfric_diagnostics.rst b/source/Development/Diagnostics/lfric_diagnostics.rst new file mode 100644 index 00000000..409a8fb0 --- /dev/null +++ b/source/Development/Diagnostics/lfric_diagnostics.rst @@ -0,0 +1,388 @@ +.. ----------------------------------------------------------------------------- + (c) Crown copyright Met Office. All rights reserved. + The file LICENCE, distributed with this code, contains details of the terms + under which the code may be used. + ----------------------------------------------------------------------------- + +.. _lfric_diag: + +LFRic Atmosphere Diagnostics +============================ + +This section describes changes required to add a new diagnostic to the LFRic +atmosphere model. If a diagnostic is being added to an existing science +scheme, it may be possible to copy and adapt code for an existing diagnostic. +Excluding code to compute the new diagnostic, adding a diagnostic amounts to +just three small changes. + +.. topic:: Checklist for experienced developers + + For experienced or confident users adding a diagnostic to an existing + science section, follow these steps: + + #. Add a new record to the field definition file. Most diagnostic fields are + defined in the ``lfric_atm/metadata/field_def_diags.xml`` file. It can + be based on an existing diagnostic if a similar one is available. + #. Add lines to initialise the field holding the diagnostic in the + appropriate diagnostics initialisation module. Add the field to the + procedure argument list. Typically, the modules are named after the + science section with the suffix ``_diags_mod.f90``. + #. Add lines to write out the diagnostic field in the output procedure held + in the same module. + #. In the algorithm that calls the initialise and write routines, add the + code to compute the new diagnostic, including passing the field down to + any algorithms or kernels involved in the computation. + #. If the diagnostic has not been requested, then the field will not be + fully initialised. Therefore, include checks to ensure such diagnostics + are not computed. + +Overview +-------- + +The diagnostic system for the LFRic atmosphere model uses the XIOS library. +XIOS inputs and outputs are configured using an ``iodef.xml`` file. An +``iodef.xml`` file for a given LFRic atmosphere model setup includes +definitions of all possible prognostics and diagnostics and, separately, a set +of diagnostic output requests. + +Field definitions are held in various files found in the ``lfric_atm/metadata`` +directory. Diagnostic requests are separate as they are part of a particular +application configuration. The ``iodef.xml`` file includes all these files by +specifying links to them. + +Any field defined in the XML file can be selected for output to a diagnostic +file. XIOS supports various post-processing operations that can be specified +in the diagnostic request to generate, for example, a daily mean of a field. +Field requests can be grouped to be output to different file streams. + +.. topic:: Comparing with the Unified Model STASH system + + Users familiar with the Unified Model STASH system will know that for each + available diagnostic, multiple output requests can be made, each of which + can undergo different time or spatial processing and can be sent to a + different file. Further, the UM system supports a graphical user interface + that can edit the lists of requests for a given application. + + XIOS has similar and more extensive processing capabilities, but at the + moment, no user interface support has been provided for users of the LFRic + atmosphere model. Requests have to be constructed by editing the XML files + included in the ``iodef.xml`` file for a given application configuration. + +XIOS is a highly complex and flexible parallel IO system with post-processing +capabilites, including the ability to apply time and spatial processing as +well as combine multiple fields. It is developed at IPSL. + +.. caution:: + + While it would be possible to write a set of complex diagnostic requests + using all the features of XIOS, people should be cautious because future + developments of the LFRic atmosphere user environment may not support all + the flexibility required to reconstruct the request. Furthermore, the + underlying XIOS software is complex, and it is possible to create a complex + diagnostic request that is unreliable or that imposes high computation + costs on the application. + +Permitted diagnostic fields +--------------------------- + +Diagnostic fields can only be created for the following types of LFRic +atmosphere fields: + +* A :math:`\mathbb{W}_{3}` field representing data, typically scalar + quantities, at the centre of each 3D model cell. +* A :math:`\mathbb{W}_{theta}` or :math:`\mathbb{W}_{2v}` field both + representing data at the centre of the bottom and top face of each 3D model + cell (noting that a cell shares the data point on its top face with the cell + above). The :math:`\mathbb{W}_{3}` fields represent scalar quantities, and + the :math:`\mathbb{W}_{2v}` represent vectors or fluxes. +* A :math:`\mathbb{W}_{2h}` field representing data, typically vectors or + fluxes, at the centre of the faces around the side of each cell(noting that + neighbouring cells share these faces and data points). +* A single-level field. +* A "multidata" field. Some types of single level + :math:`\mathbb{W}_{3}` field store more than one quantity, such as a data + point for each vegetation type or for different radiation bands. Supported + multidata fields are defined in the ``metadata/axis_def_main.xml`` file. + +Detailed steps for adding a new diagnostic +------------------------------------------ + +To add a diagnostic for potential output from the LFRic atmosphere model, +follow these steps: + +#. Create a ``field`` record in the relevant XML file that is used to create + the XIOS ``iodef.xml`` file. +#. Add code to initialise a model field to hold the diagnostic. For existing + science schemes it may be possible to add the field initialisation code to + an existing module. This step is not required if the diagnostic is obtained + from an existing field such as from a prognostic field passed into the + algorithm. +#. Add code to write out the field. + +Once the above steps are complete, code can be added to compute the diagnostic. +It is possible to check whether a diagnostic has been requested for output. If +a diagnostic has not been requested, and if it is not required for other +purposes, then the model must avoid computing its value. + +Several science schemes have dedicated modules to initialise and output +diagnostics (steps 2 and 3 of the above list). See the modules in +``um_physics_interface/source/diagnostics/``. + +The following sections describe each of the steps in more detail. + +Creating a field record +~~~~~~~~~~~~~~~~~~~~~~~ + +Diagnostics that are computed every time-step should normally be defined in the +``field_def_diags.xml`` file. + +.. topic:: Other field definition files + + Other field definition files include the ``lfric_dictionary.xml`` file which + lists prognostic fields (some of which may also be output as diagnostic + fields); the ``field_def_initial_diags.xml`` file holding definitions of + diagnostics that optionally can be output before the first time-step has + run; and the ``field_def_lbc_diags.xml`` file that supports reading and + writing of Lateral Boundary Conditions fields. + +A field definition looks like this: + +.. code-block:: xml + + + +The components of this definition are: + +* The ``id`` string is used in the model code to identify the diagnostic. + The naming convention used by the LFRic atmosphere is the section name + followed by a double-understroke followed by a descriptive name. +* The name and long name are only seen in the diagnostic output. The names + may be formally assigned such as by the CF naming convention. In this + case, the name is the same as the suffix of the ID, but it is not always + so. +* The units should be SI units. Again, these are only seen in the diagnostic + output file. +* The ``grid_ref`` attribute of this definition describes the domain of the + field. The example field above is represented in the model as + a :math:`\mathbb{W}_{theta}` field. Other field types have different + attributes as shown in the following table. + ++-----------------------------------+----------------------------------------+ +| Model field type | Domain attributes | ++===================================+========================================+ +| :math:`\mathbb{W}_{3}` | ``grid_ref="half_level_face_grid"`` | ++-----------------------------------+----------------------------------------+ +| :math:`\mathbb{W}_{theta}` | ``grid_ref="full_level_face_grid"`` | ++-----------------------------------+----------------------------------------+ +| :math:`\mathbb{W}_{2v}` | ``grid_ref="full_level_face_grid"`` | ++-----------------------------------+----------------------------------------+ +| :math:`\mathbb{W}_{2h}` | ``grid_ref="half_level_edge_grid"`` | ++-----------------------------------+----------------------------------------+ +| Single-level field | ``domain_ref="face"`` | ++-----------------------------------+----------------------------------------+ +| Multi-data field | ``domain_ref="face"`` | +| | ``axis_ref=""`` | ++-----------------------------------+----------------------------------------+ + +A multi-data field is often called a tiled field, and contains more than one +related quantity. For multi-data fields, the ```` text would +be replaced by one of the multidata field types used in the model and defined +in the ``axis_def_main.xml`` file in the ``lfric_atm/metadata`` directory. For +example the following field is on surface tiles. + +.. code-block:: xml + + + +The number of quantities in each type of multi-data field is defined within the +application. + +Initialising the field +~~~~~~~~~~~~~~~~~~~~~~ + +Initialising the field relates to defining the LFRic function space that the +field lives on rather than initialising the values held in the field. + +If a field is not available to hold the diagnostic data, then one must be +declared local to the diagnostic routine and initialised to be the correct +field type. + +The following code is from an existing science scheme algorithm. It declares +some fields and passes them to the scheme's diagnostic initialisation +procedure. + +.. code-block:: fortran + + type( field_type ) :: soil_moisture_content + type( field_type ) :: grid_canopy_water + type( field_type ) :: throughfall + type( field_type ) :: grid_throughfall + + call initialise_diags_for_jules_soil(soil_moisture_content, & + grid_canopy_water, & + throughfall, & + grid_throughfall) + +The ``initialise_diags_for_jules_soil`` procedure calls the LFRic atmosphere +``init_diag`` function: + +.. code-block:: fortran + + soil_moisture_content_flag = init_diag(soil_moisture_content, & + 'soil__soil_moisture_content') + grid_canopy_water_flag = init_diag(grid_canopy_water, & + 'surface__grid_canopy_water') + grid_throughfall_flag = init_diag(grid_throughfall, & + 'surface__grid_throughfall') + +The ``init_diag`` function does the following steps: + +#. Checks if the diagnostic needs to be computed this time-step. +#. If the diagnostic needs to be computed, the field is initialised to the + right function space type. The function space type is determined from the + domain information in the field record that was added to the ``iodef.xml`` + file according to the table above. The name of the field is set to the + string value passed into the function. +#. If the diagnostic does not need to be computed, it still initialises the + field with metadata. But to save memory, instead of the field holding its + own data array, its data array pointer is pointed to a dummy + ``empty_real_data`` field provided by the application. Fields cannot be + left uninitialised as they would cause model failures if passed through the + PSy layer as the PSy layer will try to extract their metadata. +#. The ``init_diag`` function returns ``.true.`` if the diagnostic is needs to + be output, and ``.false.`` otherwise. + +Sometimes a diagnostic needs to be computed even when it is not required for +output because of dependencies on other diagnostics that `are` required. An +optional argument to ``init_diag`` can be used to ensure a field is properly +initialised even if there is no requirement to output the diagnostic. The +following code initialises the ``throughfall`` field if the diagnostic itself +is required `or` if the ``grid_throughfall`` diagnostic is required. + +.. code-block:: fortran + + throughfall_flag = init_diag(throughfall, 'surface__throughfall', & + activate = grid_throughfall_flag) + +although the ``throughfall`` field will be properly initialised if the +``grid_throughfall`` diagnostic is required, the ``init_diag`` function will +still return ``.false.`` if the ``throughfall`` diagnostic is not requested. +The return value is used only for deciding whether to write the diagnostic to +the IO system. + +Outputting a field +~~~~~~~~~~~~~~~~~~ + +If the ``init_diag`` function has returned ``.true.`` then the diagnostic needs +to be written to the IO system once it has been computed. + +In the LFRic atmosphere model, the output procedure for a set of diagnostics is +usually in the same module as the initialisation procedure. The flag returned +by the ``init_diag`` function is available to both procedures so can be used +to determine whether the diagnostic needs to be written out. + +.. code-block:: fortran + + if (throughfall_flag) call throughfall%write_field() + if (soil_moisture_content_flag) call soil_moisture_content%write_field() + +Note that the ``write_field`` method takes no argument here. The name passed to +XIOS is the field name provided at initialisation time. + +Computing a field +----------------- + +Between calling the function to initialise fields and outputting the +diagnostics they hold, the diagnostics are computed. + +Typically, diagnostics are computed by passing them to PSyclone built-ins and +kernels. + +When using built-ins to compute a diagnostic, it is important to avoid calling +the built-in if the ``init_diag`` routine has not fully initialised the +field. + +As noted above, the ``init_diag`` routine used by the LFRic atmosphere +associates the data in a field with an ``empty_real_data`` array if the +diagnostic is not required; all such fields are pointed to the same array held +in the application's ``empty_data_mod`` module. Within a kernel, the data +array can be checked to ensure it is `not` associated with this dummy array +prior to attempting to compute the diagnostic: + +.. code-block:: fortran + + use empty_data_mod, only : empty_real_data + + ! + + if (.not. associated(throughfall, empty_real_data) ) then + do n = 1, n_land_tile + do l = 1, land_pts + throughfall(map_tile(1,ainfo%land_index(l))+n-1) = & + fluxes%tot_tfall_surft(l,n) + end do + end do + end if + +Diagnostics from existing fields +-------------------------------- + +Sometimes, a science scheme will output a diagnostic from a field passed into +the science algorithm. In this case, there is no need to declare and +initialise a local field, but the field still needs to be sent to the +diagnostic system. + +Commonly, such fields are prognostics passed into the science scheme within a +field collection. Code like the following will obtain a pointer to the +``canopy_water`` field from the ``surface_fields`` field collection so that +its value can be computed or updated: + +.. code-block:: fortran + + type( field_collection_type ), intent(inout) :: surface_fields + type( field_type ), pointer :: canopy_water + + call surface_fields%get_field('canopy_water', canopy_water) + +By default, the ``write_field`` method passes the field name to XIOS which is +matched to the IDs in the ``iodef.xml`` file. For diagnostic output, the +field's name needs to be overridden by passing the name of the diagnostic to +the write method: + +.. code-block:: fortran + + ! Prognostic fields + call canopy_water%write_field('surface__canopy_water') + +Passing ``surface__canopy_water`` means the field will be identified as a +diagnostic rather than as the ``canopy_water`` prognostic. It will then be +written to files that include diagnostic requests with that ID. + +Note that as there is no call to ``init_diag`` for an existing field, there is +no flag to determine whether or not the field needs to be sent to the IO +system. Calling the write method on a diagnostic that was not requested is not +a problem: the underlying IO system is capable of ignoring the request. + +Initial diagnostics +------------------- + +The LFRic atmosphere diagnostic definition includes a file called +``field_def_initial_diags.xml`` that defines a set of "initial" diagnostics +which are essentially the prognostic fields before the model has started +running. If requested (by setting ``write_diag=.true`` in the namelist +configuration) these fields are written out as a group at the very start of a +run just after reading the starting conditions into the model prognostic +fields. + +The initial diagnostics may be useful to analyse issues where it is believed +one of the fields is wrong just after it is read in. + +If a new field is added to the start dump, then a new record can be added to +the XML file which is a copy of the prognostic record(typically included in +the ``lfric_dictionary.xml`` file), but with its ID prefixed with ``init_``. +Explicit code to write the prognostic field as a diagnostic is added to the +``gungho_diagnostics_driver_mod`` module. diff --git a/source/Development/Diagnostics/um_stashmaster.rst b/source/Development/Diagnostics/um_stashmaster.rst new file mode 100644 index 00000000..6da602e6 --- /dev/null +++ b/source/Development/Diagnostics/um_stashmaster.rst @@ -0,0 +1,82 @@ +.. ----------------------------------------------------------------------------- + (c) Crown copyright Met Office. All rights reserved. + The file LICENCE, distributed with this code, contains details of the terms + under which the code may be used. + ----------------------------------------------------------------------------- + +.. _stash: + +STASH [#f1]_ +============ + +Information on every diagnostic available to the model is stored in a single +file named ``STASHmaster_A``, which is read into the model at the start of the +run. + +The UM's ``STASHmaster_A`` and associated help text file +``STASHmaster-meta.conf`` are available in your branch at +``vnXX.Y_/rose-meta/um-atmos/HEAD/etc/stash/STASHmaster/``. + +.. note:: + + When running the UM rose stem suite, the suite will automatically use the + ``STASHmaster_A file`` from your branch when testing your code. + +The following principles apply when altering the STASHmaster: + +* If you add a new diagnostic to the ``STASHmaster_A`` file then you **must** + also add to the stash master help text in :ref:`stashmaster-meta`. +* If you are altering the stashmaster, this may be referred to the FFPP + governance board by the sci/tech or code reviewers - see the STASH entry + guidelines. +* If your change has new stash items or changed/added attributes as an option + code, versions mask etc., then first you have to get them reserved and + recorded (published) on the reservation web page STASH/ReservedCodes +* Note that every reservation should be linked to an Issue with the correct + explanation and a milestone. This rule applies to all stash related tables + placed on this page. +* Although reservations could be some kind of self-service, contact the section + owner first nevertheless. This could help to organise new items + (when possible) in some logical groups. +* For new option code numbers contact the STASH code owner. + +.. note:: + + Complete details of the STASH system (including the syntax used in the + ``STASHmaster_A`` file) can be found in `UMDP C04 + `__ + +.. _stashmaster-meta: + +STASHmaster-meta.conf +--------------------- + +If you are adding a new UM STASH diagnostic you must also add help text to the +STASHmaster-meta.conf. This will provide others with help on your diagnostic. +You will need to identify the stash entry with a ``[stashmaster:code +(xyz)]`` section header, where the xyz is the stash code in the form ``section +number * 1000 + item number``. + +Include a full name, any units and explanatory text. You should also add a +description field that matches the full name of the diagnostic. For example: + +.. code-block:: + + [stashmaster:code(1050)] + description=NO2 Dry Deposition Rate (3D) + help=NO2 Dry Deposition Rate (3D) + =moles/s + = + =This is the total dry deposition flux of NO2 in each gridbox + = + =The sum of this deposition flux over all model gridboxes gives the total + =number of moles of NO2 removed by this process per second in the + =whole model. + +This assists the model user in being able to find useful help text on their +diagnostic. + + +.. rubric:: Footnotes + +.. [#f1] This is an acronym for 'Spatial and Temporal Averaging and Storage Handling'. diff --git a/source/Development/TestSuites/jules.rst b/source/Development/TestSuites/jules.rst new file mode 100644 index 00000000..0181bc5b --- /dev/null +++ b/source/Development/TestSuites/jules.rst @@ -0,0 +1,69 @@ +.. ----------------------------------------------------------------------------- + (c) Crown copyright Met Office. All rights reserved. + The file LICENCE, distributed with this code, contains details of the terms + under which the code may be used. + ----------------------------------------------------------------------------- + +Testing JULES +============= + +JULES testing is run with the following command from the top directory of a +local clone: + +.. code-block:: + + cylc vip -z group=all -n ./rose-stem + +----- + +The JULES rose stem testing includes a range of builds using a variety of +compilers, several configurations, and rose-ana tasks to check the output. + +The output is checked for correctness both by comparing the output to a set of +stored :ref:`KGO files `. + +.. note:: + + If there are JULES changes to shared science code or + :ref:`jules-shared` metadata then these changes + will need to be tested :ref:`with the UM` and + :ref:`with LFRic Apps`. If you have access to LFRic, the + :ref:`traclog` will state whether LFRic testing is required based on the + branch diff. If you do not have LFRic access, this testing will need to + be completed by your Met Office contact. + + See :ref:`multirepo` for details on how to carry out this testing. + + .. important:: + + For **jules-shared** changes, when LFRic testing, the + changes need to be manually synced to the LFRic location. When UM + testing, this is not required as **jules-shared** is imported from the + JULES branch. + +Below is a (by no means comprehensive) set of groups that you may wish to use +on Met Office systems. + ++----------------------+----------------------------------------------------------+ +| Group | Description | ++======================+==========================================================+ ++----------------------+----------------------------------------------------------+ +| all | The complete test suite. This is run automatically | +| | every night and monitored by the SSD team. All | +| | :ref:`KGO ` changing PRs need to run this group. | ++----------------------+----------------------------------------------------------+ ++----------------------+----------------------------------------------------------+ +| loobos | A set of tests to exercise these science areas. | +| | | +| gswp2 | | +| | | +| eraint | | +| | | +| imogen | | ++----------------------+----------------------------------------------------------+ +| ex1a/azspice | All tests designed to run on the named platform. | ++----------------------+----------------------------------------------------------+ +| scripts | All of the auxillary scripts that are designed to check | +| | the code standards in ways that aren't tested by the | +| | compiler. | ++----------------------+----------------------------------------------------------+ diff --git a/source/WorkingPractices/TestSuites/lfric_apps.rst b/source/Development/TestSuites/lfric_apps.rst similarity index 64% rename from source/WorkingPractices/TestSuites/lfric_apps.rst rename to source/Development/TestSuites/lfric_apps.rst index 884d3feb..1f50cbb0 100644 --- a/source/WorkingPractices/TestSuites/lfric_apps.rst +++ b/source/Development/TestSuites/lfric_apps.rst @@ -1,53 +1,50 @@ +.. ----------------------------------------------------------------------------- + (c) Crown copyright Met Office. All rights reserved. + The file LICENCE, distributed with this code, contains details of the terms + under which the code may be used. + ----------------------------------------------------------------------------- + .. _lfric_apps_test: Testing LFRic Apps ================== Rose stem: - LFRic Apps testing uses rose-stem and is run with the following commands - from a working copy: - - .. code-block:: - export CYLC_VERSION=8 - rose stem --group=developer - cylc play - cylc gui - -Local testing: - Alternatively, a single application can be built and run locally using - `these instructions `_ + The LFRic Apps rose-stem test suite can be run from the top directory of a + local clone: - This test does not use rose or cylc and is particularly useful for - checking for compile errors while developing. + .. code-block:: shell ------ + cylc vip -z group=developer -n ./rose-stem -.. important:: - - When specifying the lfric_core source the lfric_core revision **must** be updated in ``dependencies.sh``. +Local testing: - * If setting the source to an fcm URL, the mirror (``.xm_``) needs to be used and the revision can either be blank (for latest commit) or any valid revision for that branch. - * If setting the source to a Working Copy, the hostname needs to be provided (as Hostname:Path) and the revision must be blank. + Alternatively, a single application can be built and run locally using + `these instructions + `__ - For more details, see :ref:`multi-repo_testing`. + This test does not use rose or cylc and is particularly useful for checking + for compile errors while developing. Rose stem --------- -The LFRic Apps rose stem includes a range of tests to exercise all the applications -stored in this repository, using multiple compilers, and checksum and plot tasks to -confirm the outputs. + +The LFRic Apps rose stem includes a range of tests to exercise all the +applications stored in this repository, using multiple compilers, and checksum +and plot tasks to confirm the outputs. .. tip:: - For LFRic Apps it is possible to update the checksum files on your branch as - you progress your development to aid with testing. Details on how to do this - are on the :ref:`KGO page `. + For LFRic Apps it is possible to update the checksum files on your branch + as you progress your development to aid with testing. Details on how to do + this are on the :ref:`KGO page `. -Below is a (by no means comprehensive) set of groups that you may wish to use on -Met Office systems. Note that there is a lot of overlap between these groups, -and that you can specify more than one at once, e.g. ``--group=developer,gungho_model``. +Below is a (by no means comprehensive) set of groups that you may wish to use +on Met Office systems. Note that there is a lot of overlap between these +groups, and that you can specify more than one at once, e.g. +``-z group=developer,gungho_model``. +--------------------+----------------------------------------------------------+ | Group | Description | @@ -59,7 +56,7 @@ and that you can specify more than one at once, e.g. ``--group=developer,gungho_ | all | The complete test suite, including all longer runs and | | | less commonly used configs. This is run automatically | | | every week and monitored by the SSD team. All | -| | :ref:`KGO ` changing tickets need to run this group.| +| | :ref:`KGO ` changing PRs need to run this group. | +--------------------+----------------------------------------------------------+ +--------------------+----------------------------------------------------------+ | build | Compile tasks for all applications and science areas | @@ -68,7 +65,7 @@ and that you can specify more than one at once, e.g. ``--group=developer,gungho_ +--------------------+----------------------------------------------------------+ | integration_tests | Integration tests for all applications and science areas | +--------------------+----------------------------------------------------------+ -| xc40/spice/monsoon | All tests designed to run on the named platform. | +| ex1a/azspice | All tests designed to run on the named platform. | +--------------------+----------------------------------------------------------+ | scripts | All of the auxillary scripts that are designed to check | | | the code standards in ways that aren't tested by the | @@ -76,8 +73,8 @@ and that you can specify more than one at once, e.g. ``--group=developer,gungho_ +--------------------+----------------------------------------------------------+ As well as these general groups, each area in ``/applications`` and -``/science`` have a set of specific groups that are structured as below, -with ``name`` matching the directory name of the area. +``/science`` have a set of specific groups that are structured as +below, with ``name`` matching the directory name of the area. +--------------------+----------------------------------------------------------+ | Group | Description | diff --git a/source/Development/TestSuites/lfric_core.rst b/source/Development/TestSuites/lfric_core.rst new file mode 100644 index 00000000..efc288eb --- /dev/null +++ b/source/Development/TestSuites/lfric_core.rst @@ -0,0 +1,39 @@ +.. ----------------------------------------------------------------------------- + (c) Crown copyright Met Office. All rights reserved. + The file LICENCE, distributed with this code, contains details of the terms + under which the code may be used. + ----------------------------------------------------------------------------- + +.. _lfric_core_test: + +Testing LFRic Core +================== + +.. note:: + + At the LFRic Apps vn2.0 release, the cylc7 LFRic Core suite was deprecated + and the make test-suite functionality removed. Only the cylc8 suite is now + maintained. + +LFRic testing can be launched from the top directory of a local clone, + +.. code-block:: shell + + cylc vip -z group=developer -n ./rose-stem + +----- + +The minimum requirement for testing a branch that is to be sent for review is +that the system level developer tests pass on all the applications. These are +launched from make and utilise rose and cylc. + +While developing your change, for expediency you may want to run the tests for +only some applications. This can be done by changing the group you run, eg +``-z group=simple_diffusion``. + +The command above will launch the developer suite. You can include slightly +more testing if required by running ``-z group=all`` instead (this includes the +developer suite). + +It is also possible to run on a single platform, eg. ``-z group=ex1a``. To +select which meto EX machine is used, add ``-S USE_EX=true``. diff --git a/source/Development/TestSuites/multi-repo_testing.rst b/source/Development/TestSuites/multi-repo_testing.rst new file mode 100644 index 00000000..5dfb410d --- /dev/null +++ b/source/Development/TestSuites/multi-repo_testing.rst @@ -0,0 +1,107 @@ +.. ----------------------------------------------------------------------------- + (c) Crown copyright Met Office. All rights reserved. + The file LICENCE, distributed with this code, contains details of the terms + under which the code may be used. + ----------------------------------------------------------------------------- + +.. _multi-repo_testing: + +Multi-Repository Testing +======================== + +Multi-repository changes are expected to pass the regression tests for all the +repositories involved. To carry out the tests involved in a linked pull request +it can be helpful to refer to the :ref:`repository figure `; testing +both child and parent repositories as needed. Further details of how testing in +each repository is handled can be found on the :ref:`Testing page`. + +All Simulation Systems repositories containing a test suite will also contain a +``dependencies.yaml`` file in the top directory of the repository. This file +contains the details of all sources used by the test suite in the format (using +lfric_core as an example): + +.. code-block:: yaml + + lfric_core: + source: git@github.com:MetOffice/lfric_core.git + ref: + +The ``source`` setting sets the location of the repository on GitHub. By +default, the test suite will access GitHub repositories by using ssh, as shown +by the ``git@github.com`` part of the source. This line can be modified to point +at users fork of the repository instead, eg. +``source: git@github.com:UserName/lfric_core.git``. The ``source`` can also be a +local git clone, in which case it should take the form, +``source: :/path/to/clone``. + +The ``ref`` setting takes a git tree-ish value. Common settings will be a commit +hash, a tag or a branch name as demostrated by the examples below. At +release, the refs will be tags and will be changed to the long form of the relevant +commit hash as part of linked pull requests. + +If left blank the behaviour depends on the source: + +* **a GitHub source:** the Head of the repositories default branch will be used. +* **a local clone:** the state of the repository at source extraction time will be used. + It is recommened to set a ref when setting the source to a local clone. That way + if you switch branches in the clone, the correct branch for testing will be used. + +Various different configurations of an lfric_core source are shown below with an +explanation of each, + +.. code-block:: yaml + + # The upstream lfric_core repository at tag 3.0 + lfric_core: + source: git@github.com:MetOffice/lfric_core.git + ref: core3.0 + + # The upstream lfric_core repository at a specific commit hash + lfric_core: + source: git@github.com:MetOffice/lfric_core.git + ref: a1b2c3d4e5f67890abcdef1234567890abcdef12 + + # As above, but with a shortened form of the hash (7 characters in this case) + lfric_core: + source: git@github.com:MetOffice/lfric_core.git + ref: a1b2c3d + + # The upstream lfric_core repository, with the default branch (main) at Head + lfric_core: + source: git@github.com:MetOffice/lfric_core.git + ref: + + # A Users fork of the lfric_core repoistory, on branch my_branch + lfric_core: + source: git@github.com:UserName/lfric_core.git + ref: my_branch + + # A Users fork of the lfric_core repository at a specific hash + lfric_core: + source: git@github.com:UserName/lfric_core.git + ref: f9e8d7c + + # A local clone of the lfric_core repository, pointing at my_branch + lfric_core: + source: hostname:/path/to/lfric_core + ref: my_branch + + # A local clone of the lfric_core repository, pointing at a specific hash + lfric_core: + source: hostname:/path/to/lfric_core + ref: f9e8d7c + + +.. note:: + + If any of the testing shows up failures then there are two possible ways to + proceed: + + 1. The changes made should be re-written to avoid breaking the dependant + repositories + + 2. The changes made directly affect the interface between repositories and + therefore a change is also needed to the child repository to adapt to that change. + + If you're uncertain which route to take then the Code Owners involved will + hopefully be able to advise. diff --git a/source/Development/TestSuites/ukca.rst b/source/Development/TestSuites/ukca.rst new file mode 100644 index 00000000..a400f847 --- /dev/null +++ b/source/Development/TestSuites/ukca.rst @@ -0,0 +1,23 @@ +.. ----------------------------------------------------------------------------- + (c) Crown copyright Met Office. All rights reserved. + The file LICENCE, distributed with this code, contains details of the terms + under which the code may be used. + ----------------------------------------------------------------------------- + +Testing UKCA +============ + +Changes in UKCA that touch `src/science` or `src/control/core` must be tested +with both the UM and LFRic by following the :ref:`linked pull requests +guidance `. + +For further guidance on testing and working with UKCA, including standard +suites and box models see the `UKCA trac wiki +`__. + +There also exists a small UKCA rose-stem suite, which contains a code styling +check. This can launched from the top directory of a local clone by running, + +.. code-block:: + + cylc vip -z group=scripts -n ./rose-stem diff --git a/source/WorkingPractices/TestSuites/um.rst b/source/Development/TestSuites/um.rst similarity index 68% rename from source/WorkingPractices/TestSuites/um.rst rename to source/Development/TestSuites/um.rst index 3d900178..1db2fa20 100644 --- a/source/WorkingPractices/TestSuites/um.rst +++ b/source/Development/TestSuites/um.rst @@ -1,49 +1,51 @@ +.. ----------------------------------------------------------------------------- + (c) Crown copyright Met Office. All rights reserved. + The file LICENCE, distributed with this code, contains details of the terms + under which the code may be used. + ----------------------------------------------------------------------------- + .. _um_testing: Testing the UM ============== -UM testing is run with the following command from a working copy: +UM testing is run with the following command from the top directory of a local +clone: -.. code-block:: +.. code-block:: shell - rose stem --group=developer --new + cylc vip -z group=developer -n ./rose-stem ----- -The UM rose stem testing includes a range of builds using a variety of compilers, -both reconfiguration and atmosphere tests, and rose-ana tasks to check the output. -As well as testing the UM, there are also tests for createBC, mule, Shumlib and -a variety of other utilities. +The UM rose stem testing includes a range of builds using a variety of +compilers, both reconfiguration and atmosphere tests, and rose-ana tasks to +check the output. As well as testing the UM, there are also tests for +createBC, mule, Shumlib and a variety of other utilities. The output is checked for correctness both by comparing the output to a set of stored :ref:`KGO files `, but also by running different processor -decompositions and comparing the results, and by looking at the outputs of runs -that have stared from a cold start and those that have had their state saved and -restarted. +decompositions and comparing the results, and by looking at the outputs of +runs that have stared from a cold start and those that have had their state +saved and restarted. .. tip:: - The UM rose-stem suite can take a long time to run. When in the initial stages - of developing a change choosing a single compile and run task to test with - may be helpful. e.g. + The UM rose-stem suite can take a long time to run. When in the initial + stages of developing a change choosing a single compile and run task to + test with may be helpful. e.g. - .. code-block:: + .. code-block:: shell - rose stem --group=xc40_gnu_um_rigorous_omp-n48 + cylc vip -z group=ex1a_gnu_um_rigorous_omp-n48 -n ./rose-stem -.. note:: - Changes to code in src/atmosphere may require - :ref:`testing with LFRic Apps`.If you have access to LFRic, the - :ref:`traclog` will state whether LFRic testing is required based on the branch - diff. If you do not have LFRic access, this testing will need to be completed by - your Met Office contact. See :ref:`multirepo` for details on how to carry out this testing. -Below is a (by no means comprehensive) set of groups that you may wish to use on -Met Office systems. Note that there is a lot of overlap between these groups, -and that you can specify more than one at once, e.g. ``--group=developer,jules,ukca``. +Below is a (by no means comprehensive) set of groups that you may wish to use +on Met Office systems. Note that there is a lot of overlap between these +groups, and that you can specify more than one at once, e.g. +``-z group=developer,jules,ukca``. +--------------------+----------------------------------------------------------+ | Group | Description | @@ -60,7 +62,7 @@ and that you can specify more than one at once, e.g. ``--group=developer,jules,u | all | The complete test suite, including all longer runs and | | | less commonly used utilites. This is run automatically | | | every week and monitored by the SSD team. All | -| | :ref:`KGO ` changing tickets need to run this group.| +| | :ref:`KGO ` changing PRs need to run this group. | +--------------------+----------------------------------------------------------+ +--------------------+----------------------------------------------------------+ | rigorous_compile | A build-only group that will sense-check the code for a | @@ -78,34 +80,32 @@ and that you can specify more than one at once, e.g. ``--group=developer,jules,u +--------------------+----------------------------------------------------------+ | uk_lams | Testing for the limited area models | +--------------------+----------------------------------------------------------+ -| xc40/spice | All tests designed to run on the named platform. | +| ex1a/azspice | All tests designed to run on the named platform. | +--------------------+----------------------------------------------------------+ | scripts | All of the auxillary scripts that are designed to check | | | the code standards in ways that aren't tested by the | | | compiler. | +--------------------+----------------------------------------------------------+ -.. tip:: - The `standard jobs `_ - page for each release includes details of which of ``developer``, - ``nightly`` and ``all`` a configuration is tested at. - Monsoon ------- -As of UM vn13.9, the UM is able to run on Monsoon3, the latest version hosted by the Meto EX machines. To run on here, users should follow the process laid out in the Monsoon User Guide. This involves logging onto the ``cazccylc1`` server to launch jobs. +As of UM vn13.9, the UM is able to run on Monsoon3, the latest version hosted +by the Meto EX machines. To run on here, users should follow the process laid +out in the Monsoon User Guide. This involves logging onto the ``cazccylc1`` +server to launch jobs. The UM test suite is set up to run on Monsoon with Cylc 8 by running, -.. code-block:: +.. code-block:: shell - rose stem --group=ex1a - cylc play + cylc vip -z group=ex1a -n ./rose-stem This will launch all ex1a jobs that are available to run on Monsoon. -For running suites on Monsoon, a few environment variables must be set for the builds to complete: +For running suites on Monsoon, a few environment variables must be set for the +builds to complete: * ``EX_UMDIR_OVERRIDE`` - this needs to be set to ``/projects/metoff/umdir`` * ``ECCODES_VERSION`` - this needs to be set to ``eccodes-2.34.0`` diff --git a/source/Development/developing_change.rst b/source/Development/developing_change.rst new file mode 100644 index 00000000..01326522 --- /dev/null +++ b/source/Development/developing_change.rst @@ -0,0 +1,63 @@ +.. ----------------------------------------------------------------------------- + (c) Crown copyright Met Office. All rights reserved. + The file LICENCE, distributed with this code, contains details of the terms + under which the code may be used. + ----------------------------------------------------------------------------- + +.. _development_index: + +Developing Your Change +====================== + +How a change is developed generally depends on the nature of the change and the +size of the code change: small code changes of just a few lines can be written +in minutes before being tested. For anything longer and more complex, it is +worth developing the code in small sections or units, testing that each piece +of code works before committing back to the development branch. By following +this methodology, if one aspect of the code doesn't work, there is always the +option to revert the local changes and quickly return to a checkpoint in your +development that did work. + + +.. tip:: + + Before embarking on a medium-sized or significant model change, it is + useful to have an appropriate coding plan in place. See :ref:`planning` + for further details. + +.. note:: + + Do not add AI-generated code, e.g., from GitHub CoPilot to any branches or + forks. + +Code changes should conform to the coding standards of the project. +Remember to ensure that your changes are easy to read and follow, using +comments to explain what the code is doing. + +The vast majority of changes will be based around a project's source +code, though there are other infrastructure areas involved. Rarely, the +developer may alter one of these *without* modifying the source code. + +Every change is different, but there are a few key attributes that +increase the complexity of a particular change and should be considered +carefully: + + + +.. toctree:: + :caption: Development Checklist + :maxdepth: 1 + + planning_your_change + documentation + inputs + kgo + diagnostics + rose_stem + testing + +.. important:: + + All instructions regarding code locations in this section assume you are + working in a clone of the appropriate repository. Please read + the :ref:`working_practices_index` to discover more. diff --git a/source/Development/diagnostics.rst b/source/Development/diagnostics.rst new file mode 100644 index 00000000..3acac21d --- /dev/null +++ b/source/Development/diagnostics.rst @@ -0,0 +1,29 @@ +.. ----------------------------------------------------------------------------- + (c) Crown copyright Met Office. All rights reserved. + The file LICENCE, distributed with this code, contains details of the terms + under which the code may be used. + ----------------------------------------------------------------------------- + +Diagnostic Systems +================== + +The diagnostic systems vary between projects. Please follow the guidance +for whichever system you are developing. + ++-------+----------------------+ +| UM | :ref:`STASH ` | ++-------+----------------------+ +| LFRic | :ref:`lfric_diag` | ++-------+----------------------+ + +.. + Do UKCA/SOCRATES/JULES have their own diagnostic systems and are they worth + mentioning here? CASIM does not, but the MONC model which builds CASIM does; + code is shared between them both. + + +.. toctree:: + :hidden: + + Diagnostics/um_stashmaster + Diagnostics/lfric_diagnostics diff --git a/source/Development/documentation.rst b/source/Development/documentation.rst new file mode 100644 index 00000000..e5dfa3db --- /dev/null +++ b/source/Development/documentation.rst @@ -0,0 +1,60 @@ +.. ----------------------------------------------------------------------------- + (c) Crown copyright Met Office. All rights reserved. + The file LICENCE, distributed with this code, contains details of the terms + under which the code may be used. + ----------------------------------------------------------------------------- + +.. _docs: + +Documentation +============= + +All projects have their own scientific and technical documentation. Most +notably: + ++----------------------------+-----------------------------+ +| UM Documentation Papers |`view UM`_ | ++----------------------------+-----------------------------+ +| JULES User Guide |`view JULES`_ | ++----------------------------+-----------------------------+ +| LFRic Core |`view LFRic Core`_ | ++----------------------------+-----------------------------+ +| LFRic Apps |`view LFRic Apps`_ | ++----------------------------+-----------------------------+ + +.. _view UM: https://metoffice.github.io/um_doc/ +.. _view JULES: https://jules-lsm.github.io/latest/index.html +.. _view LFRic Core: https://metoffice.github.io/lfric_core/ +.. _view LFRic Apps: https://metoffice.github.io/lfric_apps/ + +Increasingly this documentation is stored as restructuredText markdown files +alongside the code. This is then compiled using Sphinx into the webpages above. + +.. toctree:: + :maxdepth: 1 + + sphinx_docs + um_docs + jules_docs + + +LFRic Apps and Core also use doxygen to document the code and all changes +should include appropriate doxygen changes to go with them. Doxygen guidelines +are available in the `LFRic Core Documentation +`_. + +Small changes and bug fixes rarely need documentation to be updated, but when +new science is added to a project, the documentation must be updated to ensure +that it remains contemporary with the code. + +Documentation changes that are held within a repository should be documented +with issues and pull requests, are formally reviewed, and should be included +on the same branch as the code changes. Apply the `Documentation` label to your +pull request. + +.. tip:: + + Searching the relevant documentation for words related to your change is + often useful when deciding whether to update the documentation. + + diff --git a/source/Development/inputs.rst b/source/Development/inputs.rst new file mode 100644 index 00000000..612b6dcf --- /dev/null +++ b/source/Development/inputs.rst @@ -0,0 +1,161 @@ +.. ----------------------------------------------------------------------------- + (c) Crown copyright Met Office. All rights reserved. + The file LICENCE, distributed with this code, contains details of the terms + under which the code may be used. + ----------------------------------------------------------------------------- + +.. _inputs: + +Input Variables, Rose Metadata and Upgrade Macros +================================================= + +.. important:: + + New UM Ancils must be submitted to the MIAO team for approval. Please + follow their process for `Requesting New UM Ancils + `__. + +Sometimes the developer needs to alter model namelists and input variables. A +common reason is for the inclusion of a new piece of code which has to be +turned off by default. + +If a developer changes the namelist inputs they are also expected to alter the +following areas of the code: + +* The inclusion of appropriate defensive checks. +* Changes to Rose metadata. +* Upgrade macros. + +Defensive checks abort the model run if the user sets variables outside of +their expected range or a combination of variables that will lead to an error. +Most namelist modules contain a subroutine which performs this checking and +which can be modified. + +The project metadata can be found in the following locations: + +.. tab-set:: + + .. tab-item:: UM + + ``/rose-meta/um-atmos/HEAD/rose-meta.conf`` + + .. tab-item:: JULES + + ``/rose-meta/*/*/HEAD/rose-meta.conf`` + + .. tab-item:: LFRic + + ``//rose-meta/*/HEAD/rose-meta.conf`` + +In addition to the above locations, the rose metadata is centrally mirrored on +Met Office systems. This means that metadata that has been committed to +``main`` can be accessed without a clone. This may be of use when upgrading +scientific suites between versions. + +All new namelist variables need a new entry so that the metadata loads into the +Rose GUI for users to switch it on. Additionally, sometimes the metadata needs +to be modified without changing a namelist variable. Guidance for updating the +metadata :ref:`is available `. + +.. note:: + + JULES developers also need to :doc:`update the JULES documentation + ` whenever they add or remove namelist variables. + +.. important:: + + All changes which alter namelists require an upgrade macro for them to work + with the model. + +Changes to the metadata which don't involve namelist changes may or may not +require an upgrade macro. If you are unsure whether a change needs an upgrade +macro, then run the following command on your branch: + +.. code-block:: shell + + rose stem --group=scripts + +If all of the tests pass then there is no requirement to add an upgrade macro. +If any of the metadata tests fail, then the developer should add a blank +upgrade macro which contains no upgrade commands but simply points the rose +stem suite to the new metadata. The SSD team are also available to advise on +whether an upgrade macro is necessary. + +.. tip:: + + When editing metadata you should always check that the new metadata appears + as expected in the GUI, including testing that invalid settings raise + appropriate warnings. The command to open the GUI is in general: + + .. code-block:: shell + + rose edit -C rose-stem/app/APP-NAME + + Note for MetOffice Users: ``rose edit`` runs on a server, so make sure your + source can be seen from a server - i.e. not on a drive that's only available + to your local machine. + + For LFRic Apps a few extra changes are required. In your branch (your test + branch if you have an upgrade macro): + + .. code-block:: shell + + cd rose-meta + rose edit -C ../rose-stem/app/APP-NAME --no-warn version + + If you have a linked LFRic Core or Jules development with metadata changes, + you can load their metadata by adding + ``-M /path/to/working_copy/rose-meta`` to the ``rose-edit`` command. + + +Adding a new LFRic Metadata Section +----------------------------------- + +Due to the way lfric metadata is shared, if a new LFRic metadata section is +added, then a few new files are added. A new LFRic metadata section here is +defined as a new directory within an existing or new ``rose-meta`` directory. +Adding a new metadata section requires: + +* ``rose-meta/META-NAME/HEAD/rose-meta.conf`` +* ``rose-meta/META-NAME/vnX.Y/rose-meta.conf`` (where ``X.Y`` is the most + recent released version) +* ``rose-meta/META-NAME/versions.py`` +* A symlink from the top-level rose-meta directory to the new directory + (see existing ones for examples) + +The ``vnX.Y`` and ``HEAD`` metadata should be identical for this initial +change, other than any import statements, which should point at vnX.Y or HEAD +respectively. Other ``vnX.Y`` and ``versionAB_CD.py`` files shouldn't be +modified or added (these are a snapshot of the metadata at a release). + +If a new rose-stem app using the new metadata is also being added, then +a "blank" upgrade macro will also need to be added with a ``BEFORE_TAG=vnX.Y`` +and a standard ``AFTER_TAG=vnX.Y_tTTTT``. This upgrade macro will allow the +new app to be updated to the Head metadata when the branch is merged to ``main``. +The ``rose-app.conf`` for this app will require a metadata import line of +format, ``meta=META-NAME/vnX.Y``. + + +How to add an upgrade macro to your branch +------------------------------------------ + +Please see :ref:`this page ` for further information. + +.. tip:: + + When developing a change that updates the input and/or user interface, then + repeatedly running/reverting the upgrade macro on the dev branch, or + creating many test branches can be tiresome. Consider working up your + change with the new options hard-coded in until such time as you are ready + to connect up to the input for real. + +.. important:: + + **Do not** apply the upgrade macro to your dev branch prior to the review + process. Instead you must create a test branch. See :ref:`testing`. + +.. toctree:: + :hidden: + + metadata_guidance + macros diff --git a/source/WorkingPractices/jules_docs.rst b/source/Development/jules_docs.rst similarity index 50% rename from source/WorkingPractices/jules_docs.rst rename to source/Development/jules_docs.rst index f0892634..b819b0a0 100644 --- a/source/WorkingPractices/jules_docs.rst +++ b/source/Development/jules_docs.rst @@ -1,119 +1,34 @@ +.. ----------------------------------------------------------------------------- + (c) Crown copyright Met Office. All rights reserved. + The file LICENCE, distributed with this code, contains details of the terms + under which the code may be used. + ----------------------------------------------------------------------------- + .. _jules_docs: -Building and editing the JULES User Guide -========================================= -The JULES User Guide is built using the Sphinx Documentation Generator. The -documentation is written in plain text files using a markup language called -reStructuredText. The source files for the JULES documentation are contained in -the `JULES GitHub repository`_. The plain text files are contained in the `source`_ -directory and have the extension ``.rst``. Sphinx can take these plain text -files and generate both HTML and PDF documentation from them (complete with -cross-referencing links, etc.). Since reStructuredText is a plain text format, -your favourite text editor is all you need to edit the JULES documentation. +The JULES User Guide +==================== + +The JULES User Guide is built using the :ref:`Sphinx Documentation Generator +`. The source files for the JULES documentation are contained +in the `JULES GitHub repository`_, with the plain text files contained in the +`source`_ directory. .. _JULES GitHub repository: https://github.com/jules-lsm/jules-lsm.github.io .. _source: https://github.com/jules-lsm/jules-lsm.github.io/tree/master/user_guide/doc/source -The JULES User Guide uses some custom extensions to reStructuredText to allow it -to represent Fortran namelists more effectively - these are discussed in more -detail below. Other than that, the `Sphinx documentation`_ is an excellent resource. - -.. _Sphinx documentation: https://www.sphinx-doc.org/en/master/ - -Checking out a copy of the JULES documentation ----------------------------------------------- -First time developers will need to clone the git repository before starting work: - -.. code-block:: text - - cd - git clone git@github.com:jules-lsm/jules-lsm.github.io.git - cd jules-lsm.github.io - -Create and checkout a new feature branch: - -.. code-block:: text - - git checkout -b - -Share the feature branch to GitHub: - -.. code-block:: text - - git push --set-upstream origin - -If new files have been created, use the ``git add `` command, then -ensure the new files (including the Sphinx sources) are tracked: - -.. code-block:: text - - git add - -Commit all staged changes: - -.. code-block:: text - - git commit -m "" - -Share the changes to GitHub: - -.. code-block:: text - - git push - -Create a pull request ---------------------- - -Create either a PR or, if the changes aren't quite ready for review, a draft -PR, see `GitHub - Creating a pull request`_ - -.. _GitHub - Creating a pull request: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/creating-a-pull-request - -Ensure that your JULES Trac ticket includes a link to the GitHub -pull request containing your documentation change. - -Building the JULES User Guide ------------------------------ -For first time users, please create the production environment to build the -documentation. From the top level of the repository run: - -.. code-block:: text - - conda env create -f environment.yml - -Activate the environment: - -.. code-block:: text - - conda activate jules-user-guide - -Move to the correct directory: - -.. code-block:: text - - cd /jules-user-guide-test/doc +The JULES User Guide uses some custom extensions to reStructuredText to allow +it to represent Fortran namelists more effectively - these are discussed in +more detail below. -Run ``make`` to build the documentation: - -To build and view the HTML documentation: - -.. code-block:: text - - make html - firefox build/html/index.html - -To build and view the PDF documentation: - -.. code-block:: text - - make latexpdf - evince build/latex/JULES_User_Guide.pdf reStructuredText Extension for Fortran Namelists ------------------------------------------------ -The JULES User Guide uses a custom extension to reStructuredText to allow a more -natural expression of Fortran namelists (see `user_guide/doc/sphinxext/sphinx_nml_domain.py`_ -if you are interested in the implementation). + +The JULES User Guide uses a custom extension to reStructuredText to allow a +more natural expression of Fortran namelists +(see `user_guide/doc/sphinxext/sphinx_nml_domain.py`_ if you are interested in +the implementation). .. _user_guide/doc/sphinxext/sphinx_nml_domain.py: https://github.com/jules-lsm/jules-lsm.github.io/blob/master/user_guide/doc/sphinxext/sphinx_nml_domain.py @@ -126,8 +41,8 @@ To begin documenting a namelist, the directive .. nml:namelist:: -is used. By convention, namelist names are ``UPPER_CASE``, while namelist member -names are ``lower_case``. +is used. By convention, namelist names are ``UPPER_CASE``, while namelist +member names are ``lower_case``. The ``nml:namelist`` directive does not output anything, but indicates that all subsequently declared members belong to the namelist (up until the next @@ -153,8 +68,8 @@ using the directive The white-space (indentation and blank lines) is very important here. The ``:permitted:`` annotation is optional, and can be omitted if any value is acceptable. If the member has no default value, ``:default: None`` should be -used. The description of the namelist member can contain any valid reStructuredText -markup, as long as it is indented correctly. +used. The description of the namelist member can contain any valid +reStructuredText markup, as long as it is indented correctly. The final directive used to document namelists is: @@ -168,9 +83,9 @@ The final directive used to document namelists is: .. nml:member:: -``nml:group`` is used to group logically related members within a namelist. -Any number of members can be contained within it, but they must be indented. -Any un-indented members end the group. +``nml:group`` is used to group logically related members within a namelist. Any +number of members can be contained within it, but they must be indented. Any +un-indented members end the group. For an example of how ``nml:group`` might be used, see the documentation of ``JULES_INPUT_GRID`` in `model_grid.nml`_. To see how the nml:group directive @@ -180,11 +95,13 @@ is rendered, see `JULES_INPUT_GRID namelist members`_. .. _JULES_INPUT_GRID namelist members: https://jules-lsm.github.io/latest/namelists/model_grid.nml.html#jules-input-grid-namelist-members Note - If you are adding a completely new namelist then the namelist name also -needs to be added to the contents page in source/namelists/contents.rst in order -for it to be included in the build. +needs to be added to the contents page in source/namelists/contents.rst in +order for it to be included in the build. + Cross-referencing namelists and namelist members ------------------------------------------------ + The custom reStructuredText extension for Fortran namelists also provides facilities for easily cross-referencing namelists and namelist members from anywhere in the User Guide. @@ -202,20 +119,23 @@ Similarly, to cross-reference a namelist member: :nml:mem:`::` -So to link to the member ``l_aggregate`` of namelist ``JULES_SURFACE``, we would -use the following: +So to link to the member ``l_aggregate`` of namelist ``JULES_SURFACE``, we +would use the following: .. code-block:: text - This is some text, with a link to :nml:mem:`JULES_SURFACE::l_aggregate` embedded. + This is some text, with a link to :nml:mem:`JULES_SURFACE::l_aggregate` + embedded. The cross-references are rendered as hyperlinks in the HTML version, and link to different parts of the document in the PDF version. + Checking for broken hyperlinks ------------------------------- + One can test whether there are broken hyperlinks in the user guide by running -.. code-block:: text +.. code-block:: shell - make linkcheck \ No newline at end of file + make linkcheck diff --git a/source/Development/kgo.rst b/source/Development/kgo.rst new file mode 100644 index 00000000..5de475f5 --- /dev/null +++ b/source/Development/kgo.rst @@ -0,0 +1,107 @@ +.. ----------------------------------------------------------------------------- + (c) Crown copyright Met Office. All rights reserved. + The file LICENCE, distributed with this code, contains details of the terms + under which the code may be used. + ----------------------------------------------------------------------------- + +.. _kgo: + +Known Good Output (KGO) +======================= + +Normally it is to be expected that code changes regress (i.e. all prognostic +variables **and** diagnostics maintain the same answers with your code +included but switched off). + +Usually, a code change does not regress when either a bug is a discovered and +fixed, or a science area is introduced or enhanced. Sometimes, a KGO update +may also be required to simply add a new job to the test suite or to port the +rose stem suite to new HPC architecture. + +**LFRic** KGO checksums are stored in the repository. As such with LFRic +pull requests the expectation is that you, as the developer, will include +updated KGO files as part of your branch. + +**UM and JULES** KGO output files are stored outside of the repository. Access +to this area is restricted to members of the Simulation Systems and Deployment +Team and they will update the KGO files to include your new answers as part of +the commit process. + + +KGO Update Process +------------------ + +Getting the process right for KGO changing pull requests significantly helps get +such changes onto ``main``. When preparing your change for review: + +1. Run the ``all`` rose-stem group in order to make sure that all + changes to answers have been found. + + * Include the :ref:`trac.log ` output from this testing in your + pull request summary. + +2. Add the ``kgo`` label to your pull request. + +.. tab-set:: + + .. tab-item:: LFRic Apps + + 3. Update the checksum files on your branch. To do so run rose stem and + then the following from the head of your clone. + + .. code-block:: shell + + python3 ./rose-stem/bin/update_branch_kgos.py \ + -s -w + + .. note:: + + This script requires at least python 3.9. This can be achieved + on Met Office machines by running ``module load scitools`` + + 4. If you are adding new checksums, ``git add`` the files. + + 5. You can check the new kgo updated properly by retiggering tasks in + the test suite. First retrigger ``export-source``, and then when + complete ``export-source_ex1a`` if new checksums are present there + (there is no need to retigger spice). You may need to change the + maximum window extent of the gui in order to see the succeeded + tasks. Now you can retrigger the failed checksums - these should now + pass if the kgo was updated in the clone correctly. + + 6. The changes in answers should be science reviewed by someone + familiar with the failing tests - if unsure then start with the Code + Owner for the affected application. + + + Once all the above is in place and the science and code reviews have + been completed then the Code Reviewer will merge head of ``main`` onto + your branch. If there are merge conflicts in the checksum files then + the reviewer will repeat step 3 to refresh these files. Your change is + then merged and committed to ``main``. + + .. tab-item:: UM & JULES + + 3. Contact the configuration owners of all affected configurations. A + list is provided in the trac.log . See :ref:`approvals` for details. + + Configuration owners will review the change and will either accept + the change as it is, or will request the use of a temporary + variable to switch the change off. See :ref:`templogicals` for + details of this process. + + Once all of the above is in place and the science and code reviews have + been completed the Code Reviewer will merge head of ``main`` onto + your branch, run the tests that have changed answers and use those + results to install KGO files to the filesystem. Your branch is then + merged and committed to ``main``. + +.. tip:: + + More details on the KGO update proceedures for all repositories can be + found on the :ref:`How to Commit page`. + +.. toctree:: + :hidden: + + temp_logicals diff --git a/source/Development/macros.rst b/source/Development/macros.rst new file mode 100644 index 00000000..b83031d4 --- /dev/null +++ b/source/Development/macros.rst @@ -0,0 +1,201 @@ +.. ----------------------------------------------------------------------------- + (c) Crown copyright Met Office. All rights reserved. + The file LICENCE, distributed with this code, contains details of the terms + under which the code may be used. + ----------------------------------------------------------------------------- + +.. _macros: + +Upgrade Macros +============== + +.. important:: + + When developing Upgrade Macros, they must be tested using a test branch + (see :ref:`testing`). + +To create an upgrade macro, the developer must edit a ``versions.py`` file +which is used to update the various apps in the rose stem suite to accept the +namelist changes. The upgrade macros also form the basis of the ``rose +app-upgrade`` script applied by a user wishing to upgrade from one version of +a model to the next. + +The ``versions.py`` file containing upgrade macros can be found in the +following locations: + +.. tab-set:: + + .. tab-item:: UM + + ``rose-meta/um-atmos/versions.py`` + + .. tab-item:: JULES + + ``rose-meta/jules-standalone/versions.py`` + + .. tab-item:: LFRic Core + Apps + + ``applications//rose-meta/lfric-/versions.py`` + + Variations on this theme occur, e.g. LFRic Apps science sections or + Components in LFRic Core. Metadata directories are also symlinked from a + top level ``rose-meta`` directory. + + +Within the file a blank upgrade macro will typically look like this: + +.. code-block:: python + + class vn130_tXXXX(MacroUpgrade): + + """Upgrade macro for ticket #XXXX by .""" + + BEFORE_TAG = "vn13.0" + AFTER_TAG = "vn13.0_tXXXX" + + def upgrade(self, config, meta_config=None): + """Upgrade a runtime app configuration.""" + # Input your macro commands here + return config, self.reports + +Note: The BEFORE_TAG should match the AFTER_TAG of the previous macro in the +chain. So if this is not the first macro since the release then the BEFORE_TAG +will be the version number with an added ticket number as well. For github +developments, the ticket number can be either an Issue or PR number. For +example: + +.. code-block:: python + + BEFORE_TAG = "vn13.0_t123" + + +Example of an upgrade macro +--------------------------- + +Developer Sally Smith wishes to add the logical ``l_bugfix`` to the ``&run_bl`` +namelist in the UM. To do this, she replaces the `XXXX` line in the upgrade +macro with her PR or Issue number and adds herself as the author. Within the +Python function ``upgrade``, she adds the appropriate command to include the new +logical. The macro then looks like this: + +.. code-block:: python + + class vn130_t1234(MacroUpgrade): + + """Upgrade macro for ticket #1234 by Sally Smith.""" + + BEFORE_TAG = "vn13.0" + AFTER_TAG = "vn13.0_t1234" + + def upgrade(self, config, meta_config=None): + """Add l_bugfix to namelist run_bl""" + self.add_setting(config, ["namelist:run_bl", "l_bugfix"], ".true.") + return config, self.reports + +Note that the settings added and their values are Python **strings**. This +command can then be run on a **test** branch (see :ref:`testing`). + +.. note:: + + Further information about upgrade macros can be found in the `Rose user guide + `__. + This contains information about more complex changes, such as removing + variables from namelists and changing the value that a particular variable + takes. A `tutorial + `__ + is also available. + + +Upgrade Macros in LFRic +----------------------- + +.. warning:: + + Namelist files in application example directories are not currently updated + by the Apply Macros script. This feature is intended to be introduced, but + for now, developers still need to manually update those files. + +The organisation of LFRic metadata is different from other repositories +(UM + Jules) as the metadata is stored with the Science or Application section +it is associated with and is then imported by other apps that require it. This +helps modularise the LFRic code but complicates macro chains. + +To solve this, macros in LFRic Apps are applied using a wrapper script which +will read the added macros and combine them into the ``versions.py`` files for +the apps where that metadata is imported. Therefore when adding macros, the +macro should be added in the versions.py file in the same metadata directory +as the metadata change being made. It will then be shared as appropriate by +the ``apply_macros.py`` script. + +.. tip:: + + The macro will only end up in ``versions.py`` files for metadata that is + directly imported by a rose-stem app. Therefore if adding to e.g. + Science/gungho, the macro will be deleted from that file by the script. In + this case ensure you are ready for the macros to be deleted, e.g. commit + all changes. + +For example, if a change to metadata is made in +``science/gungho/rose-meta/lfric-gungho``, the macro should be added to the +``versions.py`` file in that directory. This will then be copied to other +``versions.py`` files that import gungho metadata, e.g. lfric_atm, transport +etc. + +It is expected that all metadata changes in LFRic Core will require change to +the rose-apps in LFRic Apps, but changes to Apps must not affect Core. +Therefore, the apply_macros script requires a clone of LFRic Apps to work, but +will source it's own copy of Core if required. If your only changes are to LFRic +Core metadata, then you will also require an LFRic Apps branch and pull request. + +.. important:: + + Some complex macro commands may be dependent on the order in which they are + applied. As macros are copied by the wrapper script, the order they are + applied will always be determined by the reverse metadata import order. + For example, lfric_atm imports gungho metadata, which itself imports + components/driver. If all 3 sections have an associated macro, then the + macro commands would be applied in the order: components/driver, gungho, + lfric_atm. + +.. tip:: + + The script will read the ``dependencies.yaml`` file in your LFRic Apps clone + and will checkout a temporary copy of the LFRic Core source if required. + Some Core metadata changes will also modify the Core rose apps. In this case + make sure to also commit these changes back to the core branch. + +To add upgrade macros to LFRic the following steps can be followed: + +1. In your local LFRic Apps clone update the core source in + ``dependencies.yaml`` if you have LFRic Core changes. + +2. Add your upgrade macros. These **must** be added to the ``versions.py`` file + in the same ``rose-meta`` directory as the metadata being changed. + +3. Run the Upgrade Macro script in a test branch(see :ref:`testing`). This is + located in the `SimSys_Scripts GitHub repo + `__ (at the MetOffice an up to + date clone is available in $UMDIR/SimSys_Scripts). The syntax for running is: + + .. code-block:: shell + + export CYLC_VERSION=8 + + SimSys_Scripts/lfric_macros/apply_macros.py vnX.Y_tZZZZ \ + [--apps=/path/to/apps] [--core=/path/to/core] [--jules=/path/to/jules] + +.. important:: + + **Test branches must be used for running the Apply Macros script. Do not + commit the changes made by apply_macros.py to a Dev Branch** + +The Apps, Core and Jules options are paths to sources for each of these. Apps +will default to the present location (so it is recommended to launch from an +Apps clone). Core and Jules will default to reading the ``dependencies.yaml`` +file in the Apps source if not provided. + +The ``vnXX.Y_tTTTT`` option must match the After Tag of your upgrade macro. +When setting this, the version is the last released version of LFRic Apps. If +it's a linked Apps-Core PR, then set the ticket number based on the Apps +Issue or PR. This avoids potential ticket number clashes between the +repositories. diff --git a/source/Development/metadata_guidance.rst b/source/Development/metadata_guidance.rst new file mode 100644 index 00000000..4599ec7e --- /dev/null +++ b/source/Development/metadata_guidance.rst @@ -0,0 +1,197 @@ +.. ----------------------------------------------------------------------------- + (c) Crown copyright Met Office. All rights reserved. + The file LICENCE, distributed with this code, contains details of the terms + under which the code may be used. + ----------------------------------------------------------------------------- + +.. _metadata_guidance: + +.. + This section will need some thought and revisiting after CA2 is completed. + +.. important:: + + Do **not** edit the existing ``vnX.Y`` metadata directories or the upgrade + python files (e.g. ``versionab_cd.py``) as these are a snapshot of the + metadata at release. Only the ``HEAD`` and ``versions.py`` files should be + modified. + +Some basic guidance on Rose metadata +==================================== + +It is important that the appearance of inputs across the GUI panels conform to +the same standards. This ensures that users of each model become familiar with +how the panels work and are present with a consistent set of information, such +as follows: + +.. code-block:: + + [namelist:=] + description= + help= + sort-key= + url=.nml.html#::> + etc + +where JULES namelist items should only contain the ``url=`` field and not the +``help=`` field regardless of the repository they reside in. Each input is +presented in the GUI as: + +.. code-block:: + + variable name + short description of the variable name + +Either the help text is displayed or web help opened when one clicks the +variable name. If both fields exist the Web help is opened preferentially. If +a ``url=`` is present the ``help=`` field should be removed to avoid +duplication. + +.. _shared-namelists: + +Shared namelists +---------------- + +Shared namelist items, or items which exist in more than one repository, should +have identical metadata regardless of the repository where they reside +(e.g. JULES items in the UM metadata). The only caveats are where a +``trigger`` or a ``fail-if`` reference a namelist item from the parent model +(e.g. UM). + +Shared JULES metadata is in the process of being migrated to +``rose-meta/jules-shared``, in the JULES repository. The sub-directories are +imported by **rose-meta/um-atmos** and **rose-meta/jules-standalone** and is +manually synced with a copy in LFRic. Please see `Sharing JULES metadata +`__ for +more details including what should be in `jules-shared +`__ +and in `jules-standalone, jules-lfric or um-atmos +`__. +When developing shared JULES metadata, you will need :ref:`linked pull requests +`. The metadata migration is currently dictated by LFRic porting of +science, although the ultimate aim is to have a single source of truth. + +Please see the :ref:`rose config-edit example` for an +illustration of how to pick up **jules-shared** changes from a JULES working +copy. + +.. + We need to check if this is all still the case with cylc 8. + +The following sections provide some general guidance as to how to structure +your metadata. + +.. + This is largely based on how the UM does everything, so should be revisited + after the CA2 activity is finished. The following sections have been + +Number of panels +---------------- + +If you have a particularly long namelist, consider dividing up the number of +panels to ensure that the user doesn't have to deal with a particularly long +panel with lots of switches. However, switches which relate to each other +should always be on the same panel. + + +Sort Keys and triggers +---------------------- + +Please use sort-keys to order the appearance of items in your panels, otherwise +they will appear in alphabetical order. This means that sometimes items that +are triggered on and off will appear far away from the item that triggered +them. + +Please provide triggers for all variables where possible. It is much better and +cleaner to only see inputs that need to be set rather than everything. + +.. tip:: + + The GUI provides an option to un-hide triggered variables if one wants to + see them all. + +Please set ``compulsory=true`` for items and use triggers for when it is not +required. The settings of all variables will then be present, in all apps to +aid configuration management. When a variable is triggered off, it will be +commented out in the apps e.g. ``!!variable``. + +.. + I think from memory that JULES doesn't do the compulsory=true, which is + something for CA2 to look at. + +.. important:: + + If any variable (new or existing) has received a new compulsory=true + setting please use an upgrade macro to add that variable to apps with a + valid value. + +.. note:: + + While it is possible to trigger a variable based on ``AND`` logic + (e.g. where some condition on ``variable1`` and another condition on + ``variable2`` trigger ``variable3``), cumbersome triggering of this nature + is best avoided. + + It is not possible to trigger a variable based on ``OR`` logic. + +.. _metadata_changes: + +Viewing metadata changes as you go along +----------------------------------------- + +One can easily review their metadata changes with the rose config editor, +opening up an example app file. For example: + +.. code-block:: shell + + cd /rose-stem/app/um_n48_eg + rose config-edit -M /rose-meta/ + +If making **jules-shared** changes, when reviewing these changes from a +different child repository, you will first need to set the ``ROSE_META_PATH`` +system variable: + +.. code-block:: shell + + export ROSE_META_PATH=/rose-meta/ + +or add the path instead as a colon separated list: + +.. code-block:: shell + + rose config-edit -M /rose-meta/:/rose-meta/ + +then once the app opens click on the LHS appname to display the app meta panel. +Update this to HEAD rather than the version number and apply. + +Please note that if you have used an upgrade macro on the app then the meta +line at the top of the app file will have changed +(e.g. meta=um-atmos/vn11.0_t46). Since no metadata exists at this version rose +edit will produce an error saying that it cannot find it, instead it will use +the metadata in e.g. um-atmos/HEAD. Please click OK and continue. + +Your updates should now appear. + +Ensuring metadata changes are valid +----------------------------------- + +Developments to the metadata can be checked for errors by running `rose +metadata-check +`__ + +.. code-block:: shell + + rose metadata-check -C /path/to/rose-meta//HEAD + +where the ``-C`` option can be omitted if inside the directory containing the +metadata file. + +.. note:: + + If there are **jules-shared** changes then these need to be added to the + metadata path even in the JULES repository. As the metadata checker does + not have the ``-M`` option, this has to be done using the `ROSE_META_PATH` + environment variable as in the :ref:`previous example`. + + If the metadata checker returns "not a configuration metadata directory" + then this may indicate that the wrong path has been set. diff --git a/source/Development/planning_your_change.rst b/source/Development/planning_your_change.rst new file mode 100644 index 00000000..efa80453 --- /dev/null +++ b/source/Development/planning_your_change.rst @@ -0,0 +1,134 @@ +.. ----------------------------------------------------------------------------- + (c) Crown copyright Met Office. All rights reserved. + The file LICENCE, distributed with this code, contains details of the terms + under which the code may be used. + ----------------------------------------------------------------------------- + +.. _planning: + +Planning Your Change +==================== + +There's often a tendency to jump straight into a change. This is fine for small +and simple changes where the purpose of the change is well-understood. +However, for more complex changes having a plan for the change before +developing it can be very beneficial and a well-thought plan improves the +chances of a successful code change. + +Planning a change + +* should not take long +* does not require submission for a review - the final code should speak for + itself +* should help you understand the task and what it will involve +* should highlight some potential issues before development work starts + +This is also a good time to consult with code owners(and also with +configuration owners, if necessary). Use the appropriate :ref:`support` +channels. + +The following are some general hints and tips in planning code changes successfully. + +.. tip:: + + For more complex LFRic changes you can submit your plan to the Core Capability + Development team for a Design Review. + +.. seealso:: + :ref:`dos_donts` + +General Considerations +---------------------- +**Smaller is better.** Submit a separate pull request per bug fix or feature. +Avoid refactoring or reformating code that is not related to your change. +Multiple small pull requests are easier to review and more likely to be accepted +promptly. + +**Consider the complexity of your change.** For larger or more complex changes, +start by opening an issue and discussing your approach with the relevant people. +This helps avoid unnecessary work and ensures alignment with project goals. + +**Prioritise clarity over cleverness.** Code is read more often than it is +written, so make it easy to understand and maintain. If the logic is not +immediately obvious then include comments to explain your reasoning. + +**How does your proposed change fit in with the structure of the model?** Try +and make your code changes in-scope and no larger than they need to be. If you +find yourself having to edit large areas of code in other, unrelated sections +of the model purely to get your change to work, chances are that you're doing +something wrong. Please do seek advice. + +**Ensure that your code change meets coding standards** All models have various +coding standards and things to avoid, so it is useful for the developer to be +aware of these. + +* :ref:`Fortran Coding Styles ` +* `LFRic Coding Styles (which build on the above) + `__ +* `PEP 8 (Python) `__ + +**Who will SciTech Review the change?** This is a useful consideration as not +everyone who uses the repository has the knowledge or experience to review +every change that is being developed. Get in touch with your SciTech Reviewer +early in the process as they will have valuable insights that can help to shape +your change. + +**Does your change fix a bug or are you investigating a bug in the code?** If +so, be aware that any changes to answers will require a KGO update and +configuration owners to approve the change, which can take longer. Code changes +which require a change in answers and configuration owner approval should be +planned well in advance of the code review deadline to allow time for the +approvals to take place. + +**Is the code you need to alter on a single repository or is it spread over +multiple repositories?** If it's over multiple repositories you need to use +linked pull requests. See :ref:`multirepo` for further details. + +**Does similar code functionality already exist in the model?** It's a good +idea **not** to re-invent the wheel or have code duplication! Speaking to code +owners of the appropriate sections can help in this instance. + +**How will your change be tested?** Include unit or integration tests, and update +any example or demo repositories to exercise new functionality. + +Specific Tips for Scientific changes +------------------------------------ + +**Does the change add a new option or feature to the code?** If so, you +probably need a new namelist variable to switch the new option off and maintain +regression. This will also imply changes to the metadata are required and an +upgrade macro to include the switch into the upgraded configuration. + +**How are you going to prove that your change works scientifically?** It's +vital to make sure your code changes work when switched **on** and give the +same answer when the code is run over different processor configurations. +Producing a quick plot or plots to show the impact of your code and including +them on your pull request can aid your SciTech Reviewer in showing that your code +works properly. + +**Does the change need any new diagnostics to make sense of the code?** Many +changes will be able to use the existing diagnostics available, but if some +novel functionality is being developed it may require new diagnostics to be +added. The developer needs to check that new diagnostics output correctly and +look sensible. + +**Does your change need new prognostic variables including?** If so, these need +to be added and it is worth checking that these work properly. In the UM in +particular, adding prognostic variables involves editing a lot of routines and +is quite time-consuming. + + +Specific Tips for Technical changes +----------------------------------- + +**Avoid wholesale technical changes** These can be very cumbersome to review; +if it's possible, split the change into more manageable chunks. + +**How does your change affect the performance of the model?** If your change +intends to optimise code, be prepared to provide evidence of how things have +improved. + +.. + Comment: Are there any more that can be thought of? These PRs will + mostly be done by experienced developers and usually inside the Met + Office. diff --git a/source/Development/rose_stem.rst b/source/Development/rose_stem.rst new file mode 100644 index 00000000..9cd600d5 --- /dev/null +++ b/source/Development/rose_stem.rst @@ -0,0 +1,122 @@ +.. ----------------------------------------------------------------------------- + (c) Crown copyright Met Office. All rights reserved. + The file LICENCE, distributed with this code, contains details of the terms + under which the code may be used. + ----------------------------------------------------------------------------- + +Adding to Rose Stem Suites +========================== + +.. tip:: + + This page is for advice on adding new tasks to a rose-stem suite. For advice + on running the test suite following the move to GitHub, please see + :ref:`testing your change `. + +All new changes are strongly encouraged to come with an update to the rose stem +suite to protect any new functionality. Configuration owners may also wish to +update the suite to ensure that important configurations are protected by the +rose stem suite. + +.. warning:: + + If you find that you need to update all the apps in the rose stem suite to + get your change to work then you should use an upgrade macro. + See :ref:`inputs`. + + +Adding a new app +---------------- + +Adding a new app to rose-stem can look daunting, but is actually less difficult +than many other development processes as there are many examples to reference +already in the code. + +1. rose-stem/app/ + Start by creating a new app with the details of the test you wish to run. + This might be an optional configuration for an existing app, or something + completely new. + + For the UM also create a matching rose-ana app detailing the comparisons + required. + + Both of these will be easiest to copy an existing app that is similar to + what you are creating and modify from there. You can use the rose edit GUI + or manually modify the ``rose-app.conf`` files. + + .. note:: + + The app should validate (i.e. be consistent with the Rose metadata, and + produce no warnings or errors). This can be checked by running ``rose + macro --validate --no-warn version -M path/to/rose-meta`` from within + your new app directory, or selecting Metadata -> Check fail-if, + warn-if from the drop-down menu of the rose edit GUI. + + .. important:: + + If you are working in a UM branch and have **jules-shared** changes, + the JULES metadata path will also need adding. Please see the shared + metadata :ref:`guidance`. + +The next steps are site and rose-stem specific, but fall broadly into two +categories - those using a jinja2 templated design which populate runtime and +graph sections for you, such as the UM METO suite, and those which are +manually configured, such as JULES. + + +.. tab-set:: + + .. tab-item:: Templated + + 2. Task definitions + Task definitions should be added to the ``tasks.cylc`` for all sites + who will run this app. + + The tasks are defined in a dictionary format, with one entry per + configuration and all other details are handled by the templates. + It is possible to define multiple decomposition and thread options + in this single entry and comparison details are also defined + within that same dictionary entry. + + Details of the available options are listed in the template files + in ``rose-stem/templates`` for each suite and looking at existing + examples is encouraged. + + .. note:: + + LFRic Apps has a `detailed set of pages + `__ + that document the structure and options available for their + suite. + + + .. tab-item:: Manual + + 2. Task definitions + Task definitions for the following tasks should be added to the + ``runtime.cylc`` for all sites who will run this app. + + * run the app + * perform a KGO comparison + * perform housekeeping + + 3. Graph definitions + Graph definitions should be added to the graph.cylc for all sites + who will run this app. These should connect together your new tasks + created above with an appropriate build task. + +.. tip:: + + The site specific information is held in: + * JULES: rose-stem/include + * LFRic Apps & UM: rose-stem/site + +.. tip:: + + All ``.cylc`` files mentioned are frequently split into + platform specific variants depending on the complexity of the sites + suite. + + e.g. `runtime.cylc` may be spread across ``runtime-platform1.cylc`` and + ``runtime-platform2.cylc``. If a task should be run on both platform1 and + platform2 then both of these will need the task definition adding. diff --git a/source/Development/sphinx_docs.rst b/source/Development/sphinx_docs.rst new file mode 100644 index 00000000..5a409f00 --- /dev/null +++ b/source/Development/sphinx_docs.rst @@ -0,0 +1,84 @@ +.. ----------------------------------------------------------------------------- + (c) Crown copyright Met Office. All rights reserved. + The file LICENCE, distributed with this code, contains details of the terms + under which the code may be used. + ----------------------------------------------------------------------------- + +.. _sphinx: + +Sphinx Documentation +==================== +The documentation is written in plain text files using a markup language called +reStructuredText. The text files have the extension ``.rst``. Sphinx can +take these text files and generate both HTML and PDF documentation from them +(complete with cross-referencing links, etc.). Since reStructuredText is a +plain text format, your favourite text editor is all you need to edit the +documentation. + +.. tip:: + The `Sphinx documentation `_ is a + useful resource. + + +Building the documentation +-------------------------- + +Please create the production environment to build the +documentation (first time users only), and then activate it. + +.. tab-set:: + + .. tab-item:: JULES + + From the top level of the repository: + + .. code-block:: shell + + conda env create -f environment.yml + + Activate the environment: + + .. code-block:: shell + + conda activate jules-user-guide + + .. tab-item:: LFRic Apps and Core + + On the Met Office Azure Spice machine the main LFRic module environment + contains all the required packages to build the documentation. Ensure + this is loaded. + + Note there is a `style guide included in the LFRic Core documentation + `_ that should be followed for both + these repositories. + + .. tab-item:: Working Practices + + From the top level of the repository: + + .. code-block:: shell + + conda env create -f env.yml + + Activate the environment: + + .. code-block:: shell + + conda activate sphinx_doc_env + +Move to the documentation directory and run ``make`` to build the documentation: + +To build and view the HTML documentation: + +.. code-block:: shell + + make [clean] html + firefox build/html/index.html & + +To build and view the PDF documentation: + +.. code-block:: shell + + make latexpdf + evince build/latex/JULES_User_Guide.pdf diff --git a/source/Development/temp_logicals.rst b/source/Development/temp_logicals.rst new file mode 100644 index 00000000..bc82deac --- /dev/null +++ b/source/Development/temp_logicals.rst @@ -0,0 +1,154 @@ +.. ----------------------------------------------------------------------------- + (c) Crown copyright Met Office. All rights reserved. + The file LICENCE, distributed with this code, contains details of the terms + under which the code may be used. + ----------------------------------------------------------------------------- + +.. _templogicals: + +Temporary Logical Variables +=========================== + +Temporary logical variables are intended to add a layer of protection in +instances where a *bug fix* produces a **significant** change in results in a +scientific configuration. The purpose of the temporary logical is to maintain +results in scientific configurations. However, the bug fix could either be due +to a scientific or a technical issue. Temporary logicals are used primarily +for two reasons: + +#. To maintain consistency in an important scientific configuration + (e.g. global/regional atmosphere/land) when upgrading between model + versions. If the logical was not used then different model versions may + lead to very different scientific answers. + +#. To give the configuration owner time to assess the impact of a significant + change in answers on their configuration. The usual release cycle window is + often too short to fully assess the impact of anything other than a small + change. + +Neither of these statements suggest that the fix shouldn't be included --- in +fact the opposite is true. The decision as to whether to include a temporary +logical normally rests with the configuration owner, but with guidance from +the Code Reviewer and the Simulation Systems and Deployment team. In such +cases, the following guidance is followed: + +* Essential bug fixes (e.g. something which would on occasions cause the model + to crash) should be switched on by default and would **not** have a + temporary logical. + + * This includes any bug fixes which restores bit comparison across different + processor decomposition where it has been previously broken. + +* Any bug fix which does not lead to a change in answers should be switched on + by default (not with a temporary logical). + +* Small changes (within the noise) can usually be included as a :ref:`kgo` + update and would **not** usually have a temporary logical associated with + them. + +* Anything which has a large impact (especially on key variables used for + weather and climate) and which extends beyond the 'noise' of the + model **would be expected to include a temporary logical**. + +* There may be a few specific cases, where only certain platforms or builds are + affected due to a technical bug fix. + +.. important:: + + There isn't a precise definition of what 'being in the noise' consists of, + so the configuration owner should always be contacted in the first + instance to provide guidance of what is or is not important to that + particular configuration. + +.. important:: + + Remember that any new piece of science code or new scientific option would + normally be switched off by default by a variable in the most appropriate + scientific namelist. A temporary logical would not be used in this + instance. See :ref:`input variables ` for further details. + +Adding a temporary logical to the code base +------------------------------------------- + +When adding a new temporary logical, the developer must protect **only** the +portion of code which changes answers by placing an `IF` statement around it, +e.g. + +.. code-block:: fortran + + IF (l_fix_something) THEN + + ... piece of code which changes answers ... + + END IF ! l_fix_something + +The new logical should be added in a specified subroutine for short-term fixes +and **not** in a usual scientific or technical namelist. The location of the +temporary logical routine varies by repository, with not all repositories +requiring their own temporary logical subroutine. Details are as follows: + +.. tab-set:: + + .. tab-item:: UM + + ``src/control/misc/science_fixes_mod.F90`` + + .. tab-item:: JULES + + ``src/control/shared/jules_science_fixes_mod.F90`` + + .. tab-item:: LFRic Apps + + Currently reads ``science_fixes_mod.F90`` (see UM) into + ``um_physics/source/support/um_physics_init_mod.f90`` + + .. tab-item:: UKCA/CASIM/SOCRATES + + No temporary logical routine currently in place for these projects. + Consult with Code Owners or check the UM ``science_fixes_mod.F90`` for + existing temporary logicals. + +.. hint:: + + By default, the logical should be set to ``.false.``, with the fix + switched **off**, unless instructed otherwise by the Configuration Owner. + +The developer should remember to add the variable to the namelist, including +any namelist printing and reading subroutines present in the module. In +addition, they should also include a warning for when the fix is not included +in a configuration. Examples of the various components can be found by +examining existing variables in the subroutines listed in the table above. + +An upgrade macro and Rose metadata will be required to add the temporary +logical into the GUI and make it available to model users. See :ref:`inputs` +for further information. + + +After the release cycle +----------------------- + +Normally, configuration owners would be expected to switch on all temporary +logicals present as part of developing their latest configuration. This +includes any which do not impact their configuration, as it allows them to be +retired from the code base. Depending on when the next configuration is being +developed, this could be some time after the code is released. + +.. note:: + + **Very rarely**, switching on a bug fix may have an undesired impact (e.g. + it leads to the discovery of a bug elsewhere in the code). In these cases, + the configuration owner may keep the temporary logical set to ``.false.`` + until the issue is resolved and may consult with the Code Owners and the + developers of the fix for further guidance. This does not imply that the + bug fix wasn't sensible in the first place! + +Each temporary logical has a review and retention period attached to it. Once +the fix is included within the various configurations it affects, the +temporary logical should be removed from the code base. + +.. important:: + + Prior to a pull request containing a temporary logical being committed to + ``main``, the developer is expected to open a new Issue which removes the + logical after a fixed period. This acts as an memory aid that the logical + needs to be removed in due course. diff --git a/source/Development/testing.rst b/source/Development/testing.rst new file mode 100644 index 00000000..7fdf305a --- /dev/null +++ b/source/Development/testing.rst @@ -0,0 +1,229 @@ +.. ----------------------------------------------------------------------------- + (c) Crown copyright Met Office. All rights reserved. + The file LICENCE, distributed with this code, contains details of the terms + under which the code may be used. + ----------------------------------------------------------------------------- + +.. _testing: + +Testing Your Change +=================== + +Change to the Rose Stem Suite in Git +------------------------------------ + +.. _github_testing: + +With the move to git and GitHub, the test suites of the Simulation Systems +repositories will no longer use the rose-stem infrastructure, instead becoming +purely Cylc workflows. The only impact on the end user will be a change to the +commands required to launch the test suite. The contents of the test suite and +the process to add new tests will remain unchanged. The test suite +infrastructure will continue to live in a ``rose-stem`` directory, and we will +continue referring to the test suite as the ``rose-stem`` suite in these working +practices. + +Running the rose-stem suite will now directly call cylc commands with the +following syntax, + +* ``cylc vip`` - This will install and launch the test suite. If desired, it + can be replaced with separate ``install`` and ``play`` commands which would + need to be run separately. +* ``-z g=`` or ``-z group=`` - This sets the test suite groups to run, and + takes a comma separated list of groups. For example, ``-z g=developer, + lfric_atm`` will run the ``developer`` and ``lfric_atm`` groups. +* ``-S VALUE=SETTING`` - these options behave as they did before, and can be + added to the ``cylc vip`` command. See the table below for some suggestions. +* ``-S USE_MIRRORS=`` - An example of the above settings, this is newly added + with the git migration. By default this is ``false`` and remote GitHub + repositories will be accessed via ssh. If set to ``true``, local GitHub + mirrors will be used instead. This is recommended particularly for shared + accounts. +* ``-n name_of_suite`` - The new test suites will name themselves after the + directory containing the test suite. Unfortunately this is always + ``rose-stem`` so it is recommended to give the suite a name using this option. +* ``/path/to/rose-stem`` - The path to the rose-stem directory must be specified + if not launching from in that directory. + +For example, + +.. code-block:: + + cylc vip -z group=developer -S USE_MIRRORS=true -n my_rose_stem_suite ./rose-stem + +will launch the test suite with the ``developer`` group, using the GitHub +mirrors and naming it ``my_rose_stem_suite``. + +``-S`` Options (non-exhaustive): + +* ``-S USE_MIRRORS=true`` - Use local GitHub mirrors instead of ssh. +* ``-S USE_TOKENS=true`` - Authenticate with GitHub using a :ref:`personal + access token ` instead of ssh. If both this and ``USE_MIRRORS`` + are true, then the mirrors will be used instead. On Monsoon, this is + automatically set. +* ``-S USE_HEADS=true`` - Use the head of the default branch for the GitHub + source, only intended for usage in nightly testing. +* ``-S USE_EX[AB/CD/Z]=true`` - MetOffice only, specify the host machine for + EX1A jobs. +* ``-S HOUSEKEEPING=false`` - Stop housekeeping tasks from running. + +What Testing to Run +------------------- + +Every change should be thoroughly tested, using your judgement as to what this +involves based on the complexity of your change. There are three main methods +for you to choose from: + +Rose-stem: + Every repository has a rose-stem test suite that provides integration and + regression testing - and in some cases unit tests as well. Committing + changes to the ``main`` of each repository is dependant on these tests + passing; they are there to ensure the integrity of the codebase. + +Standard suites: + As well as the rose-stem tests you should also consider how best to prove + that your change does what it says it does, and not what it doesn't. There + may be other standard suites that will exercise your change in a + scientific or technical way. If you're unsure on what that is then talk to + the code owners involved for advice. + +Bespoke: + If the above hasn't satisfied you (or your SciTech reviewer) then you may + wish to consider further evaluation by creating your own tests. + + +.. toctree:: + :maxdepth: 1 + :caption: Test Suites + + TestSuites/um + TestSuites/lfric_apps + TestSuites/lfric_core + TestSuites/jules + TestSuites/ukca + TestSuites/multi-repo_testing + +Test branches & Upgrade Macros +------------------------------ + +.. tip:: + + While we continue to use ``dev`` and ``test`` branch nomenclature from fcm, + in GitHub these terms have no technical meaning and are simply a way to + distinguish between 2 branches. + +There are a few cases where testing your change will require you to make changes +to your branch that don't want committing to ``main``. To do this you can create +a test branch. This is a branch-of-branch from your development branch and +allows you to make those changes in an isolated environment while leaving your +original development clean. + +To create a test branch: + +.. code-block:: shell + + git switch -c test_branch_name [] + +If not provided ``start_point`` will default to your +current branch. + +If using a test branch then do link to this on your pull request and include the +results of this testing alongside those from your dev branch. + +.. Note:: + + If you need further updates to the dev branch which require retesting on the + test branch, you can update the test branch by merging in the dev branch. + + .. code-block:: + + git switch test_branch_name + git merge dev_branch_name + + If testing upgrade macros however, you will likely need a new test branch, + as the macros can only be applied once. + +Macros +^^^^^^ + +If you have updated the model inputs and included an upgrade macro with your +change then this macro will be run by your code reviewer as part of the commit +process. In order to prove that the upgrade macro will be successful, and to +make those new model inputs available to your tests, you should create a test +branch as described above and run the upgrade macro with one of the following +commands, noting that ``--jules-path`` is only required if you have +**jules-shared** metadata changes. Please see the shared metadata +:ref:`guidance`. + ++-------+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ +| UM | ``./admin/rose-stem/update_all.py --path=/path/to/working/copy/of/test/branch --um=vnX.X_tXXXX [--jules-path=/path/to/jules/working/copy/of/branch]`` | ++-------+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ +| JULES | ``./bin/upgrade_jules_test_apps vnX.X_tXXXX`` | ++-------+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ +| LFRic | ``apply_macros.py vnX.Y_tZZZZ [--apps=/path/to/apps] [--core=/path/to/core] [--jules=/path/to/jules]`` | ++-------+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ + +.. tip:: + + The ``apply_macros.py`` script is located in the `SimSys_Scripts GitHub + repo `__ (at meto an up to + date clone is available in $UMDIR/SimSys_Scripts). + +.. warning:: + + Please ensure that Cylc7 is used with ``update_all.py`` @vn13.5. This is + fixed at HoT and either Cylc7 or Cylc8 can be used. + +.. Note:: + + The update_all.py script suppresses warnings produced by upgrade macros. + You can test these separately by upgrading a single app. A single app can + be upgraded for testing using: + + .. code-block:: shell + + rose app-upgrade -M /path/to/rose-meta \ + -C /path/to/rose-stem/app/ -a + + where the ``-C`` option can be omitted if inside the app's directory. + + .. Important:: + + If there are **jules-shared** metadata changes these will need to be + added to the metadata path. Please see the :ref:`rose config-edit + example`. + + Please refer to `rose app-upgrade + `__ + command reference for more details. + +.. _traclog: + +trac.log +-------- + +The output of rose-stem from each repository includes a trac.log. This is a +wiki formatted file that can be copied into the pull request description as a record of +testing run. Please make sure that the results of your latest testing are +included when passing a pull request for review. + +.. code-block:: shell + + ~/cylc-run//runN/trac.log + + +.. tip:: + + If your suite has finished and no trac.log has been generated then it is + possible to do so manually using `suite_report.py + `__ + + On Met Office desktops a copy is stored locally allowing this to be done + with: + + .. code-block:: shell + + python3 $UMDIR/SimSys_Scripts/suite_report_git/suite_report_git.py -S + + If this is a regular problem then get in touch with the :ref:`SSD team + ` so we can investigate. Thanks. diff --git a/source/Development/um_docs.rst b/source/Development/um_docs.rst new file mode 100644 index 00000000..db649dd9 --- /dev/null +++ b/source/Development/um_docs.rst @@ -0,0 +1,126 @@ +.. ----------------------------------------------------------------------------- + (c) Crown copyright Met Office. All rights reserved. + The file LICENCE, distributed with this code, contains details of the terms + under which the code may be used. + ----------------------------------------------------------------------------- + +.. _umdp: + +Unified Model Documentation Papers (UMDPs) +========================================== + +The UM documentation papers and online training are maintained in the +MetOffice/um_doc repository, separate from the UM code. Changes to the +documentation will therefore require their own pull request. + +The latest documentation gets automatically built by CI. +The same action will run when a pull request is opened to ensure that the +requested changes builds correctly. The build script is available in the +repository and should be used to test that the finished documentation looks as +you intend before submitting it for review. + +Standards +--------- + +As there are standards for coding, similarly there are a few rules for the +UM Document Papers. + +#. Latex is the default file format. + +#. A pair of UMDP document classes are supplied in the um_doc repository for + use with all UMDPs to maintain a consistent style. + + * You must use one of these two document classes, no exceptions. + + * Unlike some other in-built Latex document classes the UMDP classes do not + support pass-through of options to their parent class, this is intentional + and will not be changed. + + * The preamble requires the following setup commands to be present: + + * **\\title** The title of the UMDP (Please avoid very long titles - use the + optional subtitle command instead) + + * **\\paperno** The UMDP identification number (3 characters, should match + the directory name), if creating a new UMDP please request a new + number (see below) + + * **\\umversion** The version of the UM the document is valid for (e.g. "9.2" + - This should ideally be the same as the documentation release as even + UMDPs with no changes should be reviewed each release-cycle) + + * **\\owner** The official owner and point of contact for the UMDP (This + might be different to the contributing authors who may be listed by the + optional command below) + + * The preamble also supports the following optional commands: + + * **\\subtitle** May be used to provide elaboration on the title (or a way of + shortening it if required) + + * **\\author** The list of contributing authors (which may or may not include + the UMDP owner) + + * **\\titlecontent** This is a flexible space which appears on the title page + and is left to the author's discretion - it could be used to place a + short abstract-style paragraph on the cover, to contain footnote + references to the list of authors or any other purpose. + + +#. Any package available in an unmodified TexLive (version 2018) installation + is permitted, provided it doesn't alter the page layout and other + presentational elements of the UMDP document class, or require source + commands that are too complex/difficult to maintain or for other authors to + understand (at the discretion of the Code Reviewer / UMDP Librarian/s). + +#. Make sure all files required for build are in the repository. No dependency + shall be stored outside the repository. + + * To ensure the integrity of the UMDP repository it is essential that all + the required files, source and diagrams to create the documentation are + included. + + * It is not acceptable to only provide a pdf or html link. + + * It should be possible for our partners to build UMDPs and modify them l + locally if we are to expect them to contribute to UM development. + + * There is no need to write a Makefile or other code to build each UMDP, + this is all handled by the build_umdoc.py script. However it identifies + the top-level document by the presence of the \documentclass command, so + you should never allow more than one file to contain this command. (See + other UMDPs for examples; these should be standard across UMDPs.) + + +Making Changes +-------------- + +Making changes to the UMDPs follows the same process for setting up a fork, +developing on a branch and reviewing through pull requests as outlined in the +:ref:`working_practices_index`. + +The source for the UMDP documents is in the ``source`` folder, in sub-directories +named after the UMDP number of the each document. You should not need to edit +anything outside of the ``source`` directory. + +When changing a document the ``Last Updated`` version number must be updated +to reflect the *upcoming* release. This is done by changing `\\umversion{}` in +the documents pre-amble. + +To test build the documentation: + +.. code-block:: + + ./build_umdoc.py XYZ + +* XYZ is the UMDP number of your modified document (more that one can + be specified if you wish) + +* The PDF output will be in `output/papers/umdp_XYZ.pdf` + +* Build stdout and stderr will be in `output/logs/umdp_XYX_stdout.log`. + + +.. tip:: + Further details on the build script can by found by directing a browser + to `web/build.html` diff --git a/source/FurtherDetails/ai.rst b/source/FurtherDetails/ai.rst new file mode 100644 index 00000000..34ec24ca --- /dev/null +++ b/source/FurtherDetails/ai.rst @@ -0,0 +1,33 @@ +.. ----------------------------------------------------------------------------- + (c) Crown copyright Met Office. All rights reserved. + The file LICENCE, distributed with this code, contains details of the terms + under which the code may be used. + ----------------------------------------------------------------------------- + +.. _ai: + +AI Policy +========= + +Please ensure that when using Generative AI tools, appropriate guardrails are +in place and contributions have correct attribution. For Met Office +contributors, this includes adhering to the ​Use of Generative AI policy. +Contributors from other institutions should check if their institution has +similar policies, and follow the local policy. + +Code where Generative AI tools have been used needs to have clear attribution to +meet the Met Office Generative AI policy. This includes attribution in the +​commit message and in each modified file. Any file where a modification has been +made with Generative AI assistance must have a comment immediately before the +module level docstring, containing: + +.. code-block:: + + # Some of the content of this file has been produced with the assistance of + ." + +where ```` should be replaced with the specific name of +the tool such as `` Github Copilot Enterprise`` (e.g. Met +Office Github Copilot Enterprise), ``Github Copilot Personal``, +``ChatGPT GPT-4``, etc. For Met Office contributors, Met Office Github Copilot +Enterprise is the only approved Generative AI tool. diff --git a/source/FurtherDetails/change_notes.rst b/source/FurtherDetails/change_notes.rst index 60b2b797..20d9a556 100644 --- a/source/FurtherDetails/change_notes.rst +++ b/source/FurtherDetails/change_notes.rst @@ -1,3 +1,9 @@ +.. ----------------------------------------------------------------------------- + (c) Crown copyright Met Office. All rights reserved. + The file LICENCE, distributed with this code, contains details of the terms + under which the code may be used. + ----------------------------------------------------------------------------- + .. _changes: Recent Changes diff --git a/source/FurtherDetails/code_of_conduct.rst b/source/FurtherDetails/code_of_conduct.rst index 0c8ec272..5215c92c 100644 --- a/source/FurtherDetails/code_of_conduct.rst +++ b/source/FurtherDetails/code_of_conduct.rst @@ -1,3 +1,9 @@ +.. ----------------------------------------------------------------------------- + (c) Crown copyright Met Office. All rights reserved. + The file LICENCE, distributed with this code, contains details of the terms + under which the code may be used. + ----------------------------------------------------------------------------- + .. _code_of_conduct: Code of Conduct @@ -10,4 +16,4 @@ of Met Office Simulation Systems. 2. Be tolerant of others but do not abuse the tolerance of others. 3. Respond to others constructively and in a reasonable time frame. 4. Communicate inclusively and contribute to the community. -5. Focus on outcomes, not processes. \ No newline at end of file +5. Focus on outcomes, not processes. diff --git a/source/FurtherDetails/coding_style.rst b/source/FurtherDetails/coding_style.rst new file mode 100644 index 00000000..27b69328 --- /dev/null +++ b/source/FurtherDetails/coding_style.rst @@ -0,0 +1,2405 @@ +.. ----------------------------------------------------------------------------- + (c) Crown copyright Met Office. All rights reserved. + The file LICENCE, distributed with this code, contains details of the terms + under which the code may be used. + ----------------------------------------------------------------------------- + +.. _standards: + +================== +Software standards +================== + +.. _`sec:intro`: + +Introduction +============ + +This document specifies the software standards and coding styles to be used +when writing new code files for the Met Office Unified Model. When +making **extensive** changes to an existing file a **rewrite** of the whole +file should be done to ensure that the file meets the UM coding standard and +style. **All code modifications within an existing file should follow these +standards**. + +The only exception to following these coding standards is that there is no +requirement to rewrite 'imported code' to these standards before it is +included within the UM. All new code developed within the Met Office should +follow these standards. + +Imported code; is developed as part of a collaboration project and then +proposed to be suitable for use within the UM; for example the original UKCA +code developed in academia. Collaborative developed code specifically for the +UM should meet these standards. + +Why have standards? +------------------- + +This document is intended for new as well as experienced programmers, so a few +words about why there is a need for software standards and styles may be in +order. + +Coding standards specify a standard working practice for a project with the aim +of improving portability, maintainability and the readability of code. This +process makes code development and reviewing easier for all developers +involved in the project. Remember that software should be written for people +and not just for computers! As long as the syntax rules of the programming +language (e.g. Fortran IV - 2003) are followed, the computer does not care how +the code is written. You could use archaic language structures, add no +comments, leave no spaces etc. However, another programmer trying to use, +maintain or alter the code will have trouble working out what the code does +and how it does it. A little extra effort whilst writing the code can greatly +simplify the task of this other programmer (which might be the original author +a year or so after writing the code, when details of it are bound to have been +forgotten). In addition, following these standards may well help you to write +better, more efficient, programs containing fewer bugs. + +While code style is very subjective, by standardising the style, UM routine +layout will become familiar to all code developers/reviewers even when they +are not familiar with the underlying science. + +Units +----- + +All routines and documentation must be written using SI units. Standard SI +prefixes may be used. Where relevant, the units used must be clearly stated in +both the code and the supporting UM documentation. + +Working practices +----------------- + +The preparation of new files and of changes to existing files should, meet this +UM standard documentation and must be developed following the stages outlined +in `Working Practices for UM Development +`__. + +Examples +-------- + +This document provides an example programming unit to aid the code developer. +This example meets the standards detailed within this paper, with references +to the relevant sections. + +Technical standards +------------------- + +UM code should be written in and conform to the Fortran 2003 standard; this is +supported by most `major Fortran compilers +`__. Obsolescent +language features are not permitted. The UM also requires compiler support for +Technical Specification 29113 on the Further Interoperability of Fortran with +C. This is a new feature for Fortran 2018, but is a `common extension in most +compilers +`__ +and has widespread support. + +Please note that in order to maximise portability and to avoid the use of +radically different design structures within single areas of code, some +Fortran 2003 features are excluded from use within the UM. For further details +please see `Appendix B`_. + + +Pre-processor +------------- + +In the past include files and C pre-processor were used for scientific code +section choices and passing a large list of arrays. This use has been phased +out and highly discouraged. The C pre-processor is still used to make machine +specific choices and, together with included files, to reduced code +duplication. These are all covered by this standards and style document. + +.. _example: + +How to meet the coding standards +================================ + +The following code example exhibits all that is defined as a good coding +standard and how code should be written for inclusion within the UM. + +The example is highlighted with references (section links) to the remainder of +this document which provide further details on the standard and style used. + +.. parsed-literal:: + + example_mod.F90 `S1`_ + + + ! *****************************COPYRIGHT******************************* `S2`_ + ! (C) Crown copyright Met Office. All rights reserved. + ! For further details please refer to the file COPYRIGHT.txt + ! which you should have received as part of this distribution. + ! *****************************COPYRIGHT******************************* `S2`_ + ! + ! An example routine depicting how one should construct + ! new code to meet the UMDP3 coding standards. `S2`_ + ! + MODULE example_mod `S3`_ `S4`_ `S6`_ + + IMPLICIT NONE `S7`_ + + ! Description: `S2`_ + ! A noddy routine that illustrates the way to apply the UMDP3 + ! coding standards to help code developers + ! pass code reviews. + ! + ! Method: `S2`_ + ! In this routine we apply many of the UMDP3 features + ! to construct a simple routine. The references on the RHS take the reader + ! to the appropriate section of the UMDP3 guide with further details. + ! + ! Code Owner: Please refer to the UM file CodeOwners.txt `S2`_ + ! This file belongs in section: Control + ! + ! Code description: `S2`_ + ! Language: Fortran 2003. + ! This code is written to UMDP3 standards. + ! + + CHARACTER(LEN=*), PARAMETER, PRIVATE :: ModuleName='EXAMPLE_MOD' `S14`_ + + CONTAINS `S1`_ + + ! Subroutine Interface: + SUBROUTINE example (xlen, ylen, l_unscale, input1, input2, & `S10`_ + output, l_loud_opt) + ! Description: + ! Nothing further to add to module description. `S2`_ + USE atmos_constants_mod, ONLY: r `S6`_ + USE ereport_mod, ONLY: ereport + USE parkind1, ONLY: jpim, jprb `S14`_ + USE umprintMgr, ONLY: umprint, ummessage, PrNorm `S12`_ + USE errormessagelength_mod, ONLY: errormessagelength + USE yomhook, ONLY: lhook, dr_hook `S14`_ + + IMPLICIT NONE `S7`_ + + ! Subroutine arguments + INTEGER, INTENT(IN) :: xlen ! Length of first dimension of the arrays. `S7`_ + INTEGER, INTENT(IN) :: ylen ! Length of second dimension of the arrays. + + LOGICAL, INTENT(IN) :: l_unscale ! switch scaling off. + + REAL, INTENT(IN) :: input1(xlen, ylen) ! First input array `S7`_ + + REAL, INTENT(IN OUT) :: input2(xlen, ylen) ! Second input array `S7`_ + REAL, INTENT(OUT) :: output(xlen, ylen) ! Contains the result `S7`_ + + LOGICAL, INTENT(IN), OPTIONAL :: l_loud_opt ! optional debug flag `S7`_ + + ! Local variables + INTEGER(KIND=jpim), PARAMETER :: zhook_in = 0 ! DrHook tracing entry `S7`_ `S14`_ + INTEGER(KIND=jpim), PARAMETER :: zhook_out = 1 ! DrHook tracing exit `S14`_ + INTEGER :: i ! Loop counter + INTEGER :: j ! Loop counter + INTEGER :: icode ! error code for EReport + LOGICAL :: l_loud ! debug flag (default false unless l_loud_opt is used) `S7`_ + + REAL, ALLOCATABLE :: field(:,:) ! Scaling array to fill. `S8`_ + REAL(KIND=jprb) :: zhook_handle ! DrHook tracing `S14`_ + CHARACTER(LEN=*), PARAMETER :: RoutineName='EXAMPLE' `S19`_ + CHARACTER(LEN=errormessagelength) :: cmessage ! used for EReport + CHARACTER(LEN=256) :: my_char ! string for output + + ! End of header + IF (lhook) CALL dr_hook(ModuleName//':'//RoutineName, zhook_in, zhook_handle) `S14`_ + + ! Set debug flag if argument is present + l_loud = .FALSE. + IF (PRESENT(l_loud_opt)) THEN `S7`_ + l_loud = l_loud_opt + END IF + + my_char & `S10`_ + = 'This is a very very very very very very very ' & + // 'loud character assignment' ! A pointless long character example. + icode=0 + + ! verbosity choice, output some numbers to aid with debugging `S5`_ + ! protected by printstatus>=PrNorm and pe=0 + WRITE(ummessage,'(A,I0)') 'xlen=', xlen `S12`_ + CALL umprint(ummessage, level=PrNorm, pe=0, src='example_mod') `S13`_ + WRITE(ummessage,'(A,I0)') 'ylen=', ylen + CALL umprint(ummessage, level=PrNorm, pe=0, src='example_mod') + IF (l_loud) CALL umprint(my_char, level=PrNorm, src='example_mod') + + ! Allocate and initialise scaling array `S5`_ + ! Noddy code warns user when scaling is not employed. + IF (l_unscale) THEN `S9`_ + icode = -100 ! set up WARNING message + ALLOCATE(field(1,1)) `S8`_ + cmessage = 'Scaling is switched off in run!' + CALL ereport(RoutineName, icode, cmessage) `S19`_ + ELSE + ALLOCATE(field(xlen, ylen)) `S8`_ + DO j = 1, ylen `S9`_ + DO i = 1, xlen `S9`_ + field(i, j) = (1.0*i) + (2.0*j) `S4`_ + input2(i, j) = input2(i, j) * field(i, j) + END DO + END DO + END IF + + ! The main calculation of the routine, using OpenMP. `S5`_ + !$OMP PARALLEL DEFAULT(NONE) & `S15`_ + !$OMP SHARED(xlen, ylen, input1, input2, field, output) & + !$OMP PRIVATE(i, j) `S15`_ + !$OMP DO SCHEDULE(STATIC) + DO j = 1, ylen + i_loop: DO i = 1, xlen `S9`_ + ! Calculate the Output value: + output(i, j) = (input1(i, j) * input2(i, j)) + END DO i_loop + END DO ! j loop + !$OMP END DO `S15`_ + !$OMP END PARALLEL `S15`_ + + DEALLOCATE (field) `S8`_ + + IF (lhook) CALL dr_hook(ModuleName//':'//RoutineName, zhook_out, zhook_handle) `S14`_ + RETURN + END SUBROUTINE example `S4`_ + + END MODULE example_mod `S4`_ + + + +.. _`sec:general`: + +UM programming standards; Code Layout, Formatting, Style and Fortran features +============================================================================= + +This section outlines the programming standards you should adhere to when +developing code for inclusion within the Unified Model. The rules set out in +this section aim to improve code readability and ensure that UM code is +compatible with the Fortran 2003 standard. + + +.. _`S1`: + +S1. Source files should only contain a single program unit +---------------------------------------------------------- + +- Modules may be used to group related variables, subroutines and functions. + Each separate file within the source tree should be uniquely named. + +- The name of the file should reflect the name of the programming unit. + Multiple versions of the same file should be named ``filename-#ver`` where + ``#ver`` is the section/version number (e.g. 1a,2a,2b…). For example: + + - ``.F90`` when writing a ```` + + - ``.F90`` with writing a ```` + + - ``.F90`` with ```` only if upgrading + existing subroutine since Subversion does not handle renaming of files + very well and this allows history of the file to be easily retrieved. + + This makes it easier to navigate the UM code source tree for given routines. + +- You should avoid naming your **program units** and **variables** with names + that match an intrinsic ``FUNCTION``, ``SUBROUTINE`` or ``MODULE``. We + recommend the use of unique names within a program unit. + +- You should also avoid naming your program units and variables with names that + match a keyword in a Fortran statement. + +- Avoid giving program units names that are likely to be used as variable names + elsewhere in the code, e.g. ``field`` or ``string``. This makes searching + the code difficult and can cause the code browser to make erroneous + connections between unrelated routines. + +- Subroutines should be kept reasonably short, where appropriate, say up to + about 100 lines of executable code, but don't forget there are start up + overheads involved in calling an external subroutine so they should do a + reasonable amount of work. + + +.. _`S2`: + +S2. Headers +----------- + +- All programming units require a suitable copyright header. Met Office derived + code should use the standard UM copyright header as depicted in the good + example code. Collaborative UM developed code may require alternative + headers as agreed in the collaborative agreements. e.g. UKCA code. The IPR + (intellectual property rights) of UM code is important and needs to be + protected appropriately. + +- Headers are an immensely important part of any code as they document what it + does, and how it does it. You should write as much of the header as possible + BEFORE writing the code, as this will focus your mind on what you are doing + and how you intend to do it! + +- The description of the ``MODULE`` and its contained ``SUBROUTINE`` may be the + same and thus it need not be repeated in the latter. If a ``MODULE`` + contains more than one subroutine then further descriptions are required. + +- History comments should not be included in the header or routine code. Version + control provides the history of our codes. + +- Code author names should NOT be included explicitly within the code as they + quickly become out of date and are sometimes misleading. Instead we + reference a single maintainable text file which is included within the UM + code repository. + + .. code-block:: fortran + + ! Code Owner: Please refer to the UM file CodeOwners.txt + ! This file belongs in section: + +- Example UM templates are provided with the source of this document; + subroutine, function and module templates. + + +.. _`S3`: + +S3. Free source form +-------------------- + +- All code should be written using the free source form. + +- Please restrict code to 80 columns, so that your code can be easily viewed on + any editor and screen and can be printed easily on A4 paper. + *Note that CreateBC uses a limit of 100 columns, due to the nature of + the object-orientated code.* + +- Never put more than one statement on a line. + +- Write your program in UK English, unless you have a very good reason for not + doing so. Write your comments in simple UK English and name your program + units and variables based on sensible UK English words. Always bear in mind + that your code may be read by people who are not proficient English + speakers. + + +.. _`S4`: + +S4. Fortran style +----------------- + +- To improve readability, write your code using the ALL CAPS Fortran keywords + approach. The rest of the code may be written in either lower-case with + underscores or CamelCase. This approach has the advantage that Fortran + keywords stand out. + +- To improve readability, you should always use the optional space to separate + the Fortran keywords. The full list of Fortran keywords with an optional + spaces is: + + :: + + ELSE IF END DO END FORALL END FUNCTION + END IF END INTERFACE END MODULE END PROGRAM + END SELECT END SUBROUTINE END TYPE END WHERE + SELECT CASE ELSE WHERE DOUBLE PRECISION END ASSOCIATE + END BLOCK END BLOCK DATA END ENUM END FILE + END PROCEDURE GO TO IN OUT SELECT TYPE + + Note that not all of these are approved or appropriate for use in UM code. + This rule also applies to OpenMP keywords. (See: `S15`_) + +- The full version of ``END`` should be used at all times, eg ``END SUBROUTINE + `` and ``END FUNCTION `` + +- New code should be written using Fortran 95/2003 features. Avoid non-portable + vendor/compiler extensions. + +- When writing a ``REAL`` literal with an integer value, put a 0 after the + decimal point (i.e. 1.0 as opposed to 1.) to improve readability. + +- Avoid using obsolescent features of the Fortran language, instead make use of + F95/2003 alternatives. For example, statement functions are among the list + of deprecated features in the F95 standard and these can be replaced by + ``FUNCTION``\ s within appropriate ``MODULE``\ s. + +- Do not use archaic forms of intrinsic functions. For example, ``LOG + ()`` should be used in place of ``ALOG()``, ``MAX()`` instead of ``AMAX1 + ()``, ``REAL()`` instead of ``FLOAT()`` etc. + +- Never use the ``PAUSE`` statement. + +- Never use the ``STOP`` statement, see `S19`_ + +- The standard delimiter for namelists is ``/``. In particular, note + that ``&END`` is non-standard and should be avoided. For further information + on namelists please refer to :ref:`namelists`. + +- Only use the generic names of intrinsic functions, avoid the use of + 'hardware' specific intrinsic functions. Use the latter if an only if + there is an optimisation benefit and then it must be protected by a + platform specific CPP flag `S17`_. + +.. # .. _`sec:comments`: + +.. _`S5`: + +S5. Comments and white spacing +------------------------------ + +- Always comment code! + +- Start comments with a single ``!``. The indention of whole line comments should + match that of the code. + +- Use spaces and blank lines where appropriate to format your code to improve + readability. + +- Never use tabs within UM code as the tab character is not in the Fortran + character set. If your editor inserts tabs automatically, you should + configure it to switch off the functionality when you are editing Fortran + source files. + +- Line up your statements, where appropriate, to improve readability. + +.. # .. _`sec:modules`: + +.. _`S6`: + +S6. The use of modules +---------------------- + +MODULEs are strongly encouraged as the mainstay of future UM code program +units; making use of the implicit ``INTERFACE`` checking and removing the need +for the ``!DEPENDS ON``. Argument lists within ``SUBROUTINE`` ``CALLs`` may +also shorten. + +- You are expected to ``USE , ONLY : `` and variables should + be imported from the module in which they were originally declared thus + enabling a code audit trail of variables around the UM code. + +- For code portability, be careful not to ``USE `` twice in a routine + for the same MODULE, especially where using ``ONLY``. This can lead to + compiler Warning and Error messages. + +- Where possible, module variables and procedures should be declared PRIVATE. + This avoids unnecessary export of symbols, promotes data hiding and may also + help the compiler to optimise the code. + +- The use of derived types is encouraged, to group related variables and their + use within Modules. + +- Review your use of arguments within subroutine calls, could some be + simplified by using Modules? + +- Before writing your Module, check the UM source that no one has already + created a Module to do what you want. For example do not declare a new + variable/parameter without checking if it is already available in a suitable + UM module. + +- Global type constants (e.g. :math:`g` and :math:`\pi`) should be maintained + at a high level within the UM code and not duplicated within modules at the + code section level; ``USE `` instead. + Only section specific constants should be maintained at the section level. + +- When calling another Subroutine or an External Function the use of + ``! DEPENDS ON`` directive is required within the Unified Model prior to + the ``CALL`` unless the Subroutine or Function is wrapped within a Module; + thus USE it, + + .. code-block:: fortran + + ! DEPENDS ON: gather_field_gcom + CALL gather_field_gcom(local_field, global_field, & + local_row_len, local_rows, & + global_row_len, global_rows, & + grid_type, halo_type, & + gather_pe, proc_group, & + icode, cmessage) + +- Avoid the introduction of additional ``COMMON`` blocks. Developers + should now be using ``MODULE``\ s. + +.. # .. _`sec:declare`: + +.. _`S7`: + +S7. Argument and variable declaration +------------------------------------- + +- Use IMPLICIT NONE in all program units. This forces you to declare all your + variables explicitly. This helps to reduce bugs in your program that will + otherwise be difficult to track. + +- Use meaningful variable names to aid code comprehension. + +- Variables should not use Fortran keywords or intrinsic functions for their + name. For example, a variable should not be named ``size``, because there is + already a Fortran intrinsic function called ``SIZE()`` + +- For the purposes of variable naming, "Fortran keywords or intrinsic + functions" shall refer to the set of all keywords and functions, from all + Fortran Standard versions (including all past and future versions, not just + Fortran 2003). For, example, the ``ASSIGN`` keyword was deleted in Fortran + 95, but ``assign`` still should not be used as a variable name. + +- All variables must be declared, and commented with a brief description. This + increases understandability and reduces errors caused by misspellings of + variables. + +- Use ``INTENT`` in declaring arguments as this allows for checks to be done at + compile time. + +- Arguments should be declared separately from local variables. + +- Subroutine arguments should be declared in the same order in the header as + they appear in the subroutine statement. This order is not random but is + determined by intent, variable dimensions and variable type. All input + arguments come first, followed by all input/output arguments and then all + output arguments. The exception being any ``OPTIONAL`` arguments which + should be appended to the end of the argument list. If more than one + ``OPTIONAL`` argument is used then one should also use keywords so that the + ``OPTIONAL`` arguments are not tied to a specific 'position' near the end of + the argument list. + +- As ``OPTIONAL`` arguments are possible when using ``MODULE``\ s (an interface + is required) there is no requirement in future for DUMMY arguments and glue + routines. + +- It is recommended that one uses local variables in routines which are set to + the values of optional arguments in the code if present, otherwise a default + value is used. This removes the requirement to always use ``PRESENT`` when + using the optional argument. + +- Within each section of the header, variables of a given type should be + grouped together. These groups must be declared in the order ``INTEGER``, + ``REAL``, ``LOGICAL`` and then ``CHARACTER``, with each grouping separated + by a blank line. In general variables should be declared one per line. Use a + separate type statement for each line as this makes it easier to copy code + around (you can always use the editor to repeat a line to save typing the + type statement again) and prevents you from running out of continuation + lines. + +- If an array is dimensioned by another variable, ensure that the variable is + declared first. + +- The ``EXTERNAL`` statement should not be used for subroutines although it is + allowed for functions, again for code portability. + +- Avoid the ``DIMENSION`` attribute or statement. Declare the dimension with + the declared variables which improves readability. + + Common practice + + .. code-block:: fortran + + INTEGER, DIMENSION(10,20) :: a, b, c + + Better approach + + .. code-block:: fortran + + INTEGER :: a(10, 20), b(10, 20), c(10, 20) + + +- Initialisation in the declaration of a variable should only be done after + considering whether it is to be only initialised on the first encounter of + the variable or not. Fortran automatically adds ``SAVE`` to the declaration + attribute to this type of initialisation. This is especially important in + OpenMP and when you expect the variable to be reset everytime the routine is + entered. ``POINTER``\ s are also affected so please be aware of the + effects. + +- Character strings must be declared with a length when stored in an array. + +- If an argument list has a dummy argument that makes use of incoming data + (whether or not it has an explicit ``INTENT``) and another argument + explicitly declared ``INTENT(OUT)``, do not use the same variable as the + actual argument to both dummy arguments ("aliasing"). Some compilers will + reinitialise all ``INTENT(OUT)`` variables on entry, destroying the incoming + data. + + Example subroutine: + + .. code-block:: fortran + + SUBROUTINE foo(m,n) + REAL, INTENT(IN) :: m + REAL, INTENT(OUT) :: n + + Bad practice: + + .. code-block:: fortran + + CALL foo(a,a) + + Safe approach: + + .. code-block:: fortran + + b = a + CALL foo(b,a) + +.. #.. _`sec:allocate`: + +.. _`S8`: + +S8. Allocatables +---------------- + +- When Allocating and deallocating, use a separate ALLOCATE and DEALLOCATE + statement for each array. + +- When using the ``ALLOCATE`` statement, ensure that any arrays passed to + subroutines have been allocated, even if it's anticipated that they won't be + used. + + .. code-block:: fortran + + IF (L_mcr_qrain) THEN + ALLOCATE ( mix_rain_phys2(1-offx:row_length+offx, & + 1-offy:rows+offy, wet_levels) + ELSE + ALLOCATE ( mix_rain_phys2(1,1,1) ) + END IF + + ! DEPENDS ON: q_to_mix + CALL do_something(row_length, rows, wet_levels, & + offx,offy, mix_rain_phys2 ) + +- To prevent memory fragmentation ensure that allocates and deallocates match + in reverse order. + + .. code-block:: fortran + + ALLOCATE ( A(row_length,rows,levels) ) + ALLOCATE ( B(row_length,rows,levels) ) + ALLOCATE ( C(row_length,rows,levels) ) + .... + DEALLOCATE ( C ) + DEALLOCATE ( B ) + DEALLOCATE ( A ) + +- Where possible, an ALLOCATE statement for an ALLOCATABLE array (or a POINTER + used as a dynamic array) should be coupled with a DEALLOCATE within the same + scope. If an ALLOCATABLE array is a PUBLIC MODULE variable, it is highly + desirable for its memory allocation and deallocation to be only performed in + procedures within the MODULE in which it is declared. You may consider + writing specific SUBROUTINEs within the MODULE to handle these memory + managements. + +- Always define a POINTER before using it. You can define a POINTER in its + declaration by pointing it to the intrinsic function NULL() (also see advice + in `S7`_). Alternatively, you can make sure that your POINTER is defined or + nullified early on in the program unit. Similarly, NULLIFY a POINTER when it + is no longer in use, either by using the NULLIFY statement or by pointing + your POINTER to NULL(). + +- New operators can be defined within an ``INTERFACE`` block. + +- ``ASSOCIATED`` should only be done on initialised pointers. + Uninitialised pointers are undefined and ``ASSOCIATED`` can have + different effects on different platforms. + +.. # .. _`sec:blocks`: + +.. _`S9`: + +S9. Code IF blocks, DO LOOPs, and other constructs +-------------------------------------------------- + +- The use of comments is required for both large ``DO`` loops and large + ``IF`` blocks; those spanning 15 lines or more, see `S5`_. + +- Indent blocks of code by 2 characters. + +- Use the newer forms of the relational operators for LOGICAL + comparisons: + + :: + + == instead of .EQ. + /= instead of .NE. + > instead of .GT. + < instead of .LT. + >= instead of .GE. (do not use =>) + <= instead of .LE. (do not use =<) + +- Positive logic is usually easier to understand. When using an IF-ELSE-END IF + construct you should use positive logic in the IF test, provided that the + positive and the negative blocks are about the same length. + + Common practice + + .. code-block:: fortran + + IF (my_var /= some_value) THEN + CALL do_this() + ELSE + CALL do_that() + END IF + + Better approach + + .. code-block:: fortran + + IF (my_var == some_value) THEN + CALL do_that() + ELSE + CALL do_this() + END IF + +- Where appropriate, simplify your LOGICAL assignments, for example: + + .. container:: samepage + + Common practice + + .. code-block:: fortran + + IF (my_var == some_value) THEN + something = .TRUE. + something_else = .FALSE. + ELSE + something = .FALSE. + something_else = .TRUE. + END IF + ! ... + IF (something .EQV. .TRUE.) THEN + CALL do_something() + ! ... + END IF + + .. container:: samepage + + Better approach + + .. code-block:: fortran + + something = (my_var == some_value) + something_else = (my_var /= some_value) + ! ... + IF (something) THEN + CALL do_something() + ! ... + END IF + +- Avoid the use of 'magic numbers' that is numeric constants hard wired into + the code. These are very hard to maintain and obscure the function of the + code. It is much better to assign the 'magic number' to a variable or + constant with a meaningful name and then to use this throughout the code. In + many cases the variable will be assigned in a top level control routine and + passed down via a include file or module. This ensures that all subroutines + will use the correct value of the numeric constant and that alteration of it + in one place will be propagated to all its occurrences. Unless the value + needs to be alterable whilst the program is running (e.g. is altered via I/O + such as a namelist) the assignment should be made using a ``PARAMETER`` + statement. + + .. container:: samepage + + Poor Practice + + .. code-block:: fortran + + IF (ObsType == 3) THEN + + .. container:: samepage + + Better Approach + + .. code-block:: fortran + + ...specify in the header local constant section.... + + INTEGER, PARAMETER :: SurfaceWind = 3 !No. for surface wind + + ...and then use in the logical code... + + IF (ObsType == SurfaceWind) THEN + +- Similarly avoid the use of 'magic logicals' in CALLs to subroutines. Such use + makes the code less readable and developers are required to look at the + called subroutine to find what has been set to either ``.TRUE.`` or + ``.FALSE.``. + + .. container:: samepage + + Poor Practice + + .. code-block:: fortran + + CALL Phys(.FALSE.,.TRUE.,icode) + + .. container:: samepage + + Better Approach + + .. code-block:: fortran + + ...specify in the header local constant section.... + ...meaningful logical names, perhaps base them on what is used in the called subroutine + + LOGICAL, PARAMETER :: bl_is_off = .FALSE. + LOGICAL, PARAMETER :: conv_is_on = .TRUE. + + ...and then use in the relevant subroutine calls... + + CALL Phys(bl_is_off, conv_is_on, icode) + +- **Be careful** when comparing real numbers using ``==``. To avoid problems + related to machine precision, a threshold on the difference between the + two numbers is often preferable, e.g. + + .. container:: samepage + + Common practice + + .. code-block:: fortran + + IF ( real1 == real2 ) THEN + ... + END IF + + .. container:: samepage + + Better approach + + .. code-block:: fortran + + IF ( ABS(real1 - real2) < small_number ) THEN + ... + END IF + + where small_number is some suitably small number. In most cases, a suitable + value for small_number can be obtained using the Fortran intrinsic functions + ``EPSILON`` or ``TINY``. + + The UM perturbation sensitivity project is currently in the process of + identifying coding issues that lead to excessive perturbation growth in + the model. Currently, all problems are emerging at IF tests that contain + comparisons between real numbers. Typical, real case UM examples of what + can go wrong are detailed in `Appendix C`_ of this document. + +- Loops *must* terminate with an ``END DO`` statement. To improve the clarity + of program structure you are encouraged to add labels or comments to the + ``DO`` and ``END DO`` statements. + + .. code-block:: fortran + + DO i = 1, 100 + j_loop: DO j = 1, 10 + DO k = 1, 10 + ...code statements... + END DO ! k + END DO j_loop + END DO ! outer loop i + +- ``EXIT`` statements *must* be labelled. This is both for clarity, and to + ensure consistency of behaviour. (The semantics of the ``EXIT`` statement + changes between revisions of the Fortran standard.) + + .. code-block:: fortran + + i_loop: DO i = 1, 10 + IF (i > 3) EXIT i_loop + END DO i_loop + +- Avoid the use of the ``GO TO`` statement. + + - The only acceptable use of ``GO TO`` is to jump to the end of a routine + after the detection of an error, in which case you must use ``9999`` as + the label (then everyone will understand what ``GO TO 9999`` means). + + - UM Error reporting guidance is detailed in `S19`_ + +- Avoid assigned ``GO TO``, computed ``GO TO``, arithmetic ``IF``, etc. Use the + appropriate modern constructs such as ``IF``, ``WHERE``, ``SELECT CASE``, + etc.. + +- Where possible, consider using ``CYCLE``, ``EXIT`` or a ``WHERE`` construct + to simplify complicated ``DO`` loops. + +- Be aware that logic in ``IF`` conditions can be performed in any order. So + checking that array is greater than lower bound and using that index is not + safe. + + Common approach + + .. code-block:: fortran + + DO j = 1, rows + DO i = 1, row_length + IF (cloud_level(i,j) > 0 .AND. cloud(i,j,cloud_level(i,j)) == 0.0) THEN + cloud(i,j,cloud_level(i,j)) = 1.0 + END IF + END DO + END DO + + Better approach + + .. code-block:: fortran + + DO j = 1, rows + DO i = 1, row_length + IF (cloud_level(i,j) > 0) THEN + IF (cloud(i,j,cloud_level(i,j)) == 0.0) THEN + cloud(i,j,cloud_level(i,j)) = 1.0 + END IF + END IF + END DO + END DO + +- Array initialisations and literals should use the ``[]`` form rather than the + ``(//)`` form. For example: + + .. code-block:: fortran + + INTEGER :: i_array(3) = [1,2,3] + +.. # .. _`sec:contd`: + +.. _`S10`: + +S10. Line continuation +---------------------- + +- The only symbol to be used as a continuation line marker is '``&``' at the + end of a line. It is suggested that you align these continuation markers to + aid readability. Do not add a second '``&``' to the beginning of the next + line. This advice also applies to blocks of Fortran code protected by the + OpenMP sentinel '``!$``'. The only currently allowed exception is to + continuation lines used with OpenMP directives, i.e. '``!$OMP``', where + the '``&``' marker may optionally be used. Please see section `S15`_ for + more advice on OpenMP. + +- Short and simple Fortran statements are easier to read and understand than + long and complex ones. Where possible, avoid using continuation lines in a + statement. + +- Try to avoid string continuations and spread the string across multiple lines + using concatenations (``//``) instead. + +- When calling functions or subroutines, ensure the left parenthesis is on the + same line as the subprogram's name, and not after a continuation marker. + This helps the code browser to parse the source tree correctly. + +.. # .. _`sec:fortio`: + +.. _`S11`: + +S11. Fortran I/O +---------------- + +- When calling ``OPEN``, ensure that the ``ACTION`` argument is specified. In + particular, ``ACTION='READ'`` shall be used for files that are opened only + for reading as this reduces file locking costs. + +- Don't check for the existence of a file by using ``INQUIRE`` if the only + action you'll take if the file doesn't exist is to report an error. Rather + use ``OPEN( ... , IOSTAT=icode, IOMSG=iomessage)`` and include the + ``iomessage`` in an error message if ``icode`` is non-zero. This will + capture a wider range of errors with fewer filesystem metadata accesses. + + +.. _`S12`: + +S12. Formatting and output of text +---------------------------------- + +Writing output to the "stdout" stream, commonly unit 6 in fortran must use the +provided API, which is accessible by including ``USE umPrintMgr`` in the +calling code. + +- Single string output should be written as + + .. code-block:: fortran + + CALL umprint('Hello',src='routine_name') + + where 'routine_name' is the name of the current subroutine or function. + Routines which implement DrHook (section `S14`_) will already have a + :literal:`PARAMETER \'RoutineName'` which can be used for this + purpose. + +- Multi-component output must first be written to an internal file via + ``WRITE`` statement. The ``umPrintMgr`` module provides a convenient string + for this purpose; ``umMessage``, though you may use your own. + + .. code-block:: fortran + + WRITE (ummessage,'(A,I0,A)') 'I am ', age, ' years old' + CALL umprint(ummessage,src='routine_name') + +- Avoid the use of ``WRITE (ummessage,*)`` + +- Always add formatting information to your write statements. It is important + to ensure that the output message fits within the space given. Some + compilers will pad unformatted values with leading blanks, which can greatly + increase the width of any output. Writes to internal files may cause the + program to abort if the message is longer than the string provided. + +- Use dynamic-width edit descriptors where possible, to avoid truncating + strings or failing to print integer or real values correctly: + + - Use ``A`` for character input and output, rather than e.g. ``A7``. + + - Use ``I0`` for integer output, rather than e.g. ``I3``. + + - Use ``F0.``\ :math:`n` for real output, rather than e.g. + ``F14.``\ :math:`n`. Other real edit descriptors such as ``E``, ``EN`` and + ``ES`` can also be used but do not accept a 0 field width. + + This is particularly important in any routine where missing data indicators + may be present, which will typically require a much larger width than other + data. + +- The character variable ``newline`` (from the ``umPrintmgr`` module) is + recognised as a newline if embedded in the string passed to ``umPrint``. + +- The total line length should not exceed 80 characters. Use ``newline`` or + separate calls to ``umprint`` to keep long messages easily readable. + +- ``CHARACTER`` values should not contain vertical space, nor should edit + descriptors be used for carriage control. Use ``newline`` to control + vertical space: + + .. code-block:: fortran + + WRITE(ummessage, '(A)') newline // 'This should stand out.' // newline + CALL umprint(ummessage,src='routine_name') + +- Calls to ``umPrint`` should be protected by a suitable setting of the + PrintStatus variable, see `S13`_ either with conditional logic or an + additional ``level`` argument, + + .. code-block:: fortran + + CALL umprint(ummessage,src='routine_name',level=PrOper) + +- If your output is not required from each processor protect the ``umPrint`` + either with logic, or an additional ``pe`` argument, for example, + + .. code-block:: fortran + + ! We'll only output at diagnostic level on pe0 + CALL umprint(ummessage,src='routine_name',level=PrDiag,pe=0) + +- Never use a ``FORMAT`` statement: they require the use of labels, and obscure + the meaning of the I/O statement. The formatting information can be placed + explicitly within the ``READ``, ``WRITE`` or ``PRINT`` statement, or be + assigned to a ``CHARACTER`` variable in a ``PARAMETER`` statement in the + header of the routine for later use in I/O statements. Never place output + text within the format specifier: i.e. only format information may be placed + within the ``FMT=`` part of an I/O statement, all variables and literals, + including any character literals, must be 'arguments' of the I/O routine + itself. This improves readability by clearly separating what is to be + read/written from how to read/write it. + + Common practice + + .. code-block:: fortran + + WRITE(Cmessage, & + & '("Cannot run with decomposition ",I3," x ",I3, & + & " (",I3,") processors. ", & + & "Maxproc is ",I3," processors.")') & + & nproc_EW,nproc_NS,nproc_EW*nproc_NS,Maxproc + + Better approach + + .. code-block:: fortran + + WRITE(cmessage,'(4(A,I0),A)') & + 'Cannot run with decomposition ',nproc_ew,'x',nproc_ns, & + '(',nproc_ew*nproc_ns,') processors. Maxproc is ',maxproc, & + ' processors.' + +- In order to flush output buffers, the routine ``umprintflush`` should be used + for "stdout" written via ``umprint`` and ``UM_FORT_FLUSH`` for data writtent + to any other fortran unit. These routines abstract flush operations + providing a portable interface. These are the only method of flushing that + should be used. + + +.. _`S13`: + +S13. PrintStatus +---------------- + +There are four different settings of PrintStatus used in the UM, each of which +is assigned a numeric value. There is a shorter form available for each one. +These are defined as ``PARAMETER``\ s and so can be tested using constructs +similar to: + +.. code-block:: fortran + + IF (PrintStatus >= PrStatus_Normal) THEN + +For "stdout", they can also be provided as an argument to ``umprint``. The +current value of PrintStatus is stored in the variable ``PrintStatus`` in the +aforementioned module, and set using the gui and/or input namelist. Note that +the utility executables operate at a fixed value of ``PrintStatus`` and that +output choices in code shared with these utilities will impact their +behaviour. + +The different settings are: + +- ``PrStatus_Min`` or ``PrMin`` - This setting is intended to produce minimal + output and should hence be only used for output which is required in every + run. Users running with this setting should expect to have to rerun with a + more verbose setting to diagnose any problems. Fatal error messages should + fall into this category, but otherwise it should not generally be used by + developers. + +- ``PrStatus_Normal`` or ``PrNorm`` - The "standard" setting of PrintStatus. + Messages with this setting should be important for all users in every run. + Information output using this setting should summarise the situation - more + detailed information should be protected by ``PrStatus_Diag`` instead. + +- ``PrStatus_Oper`` or ``PrOper``- Slightly more detailed than + ``PrStatus_Normal``, this is intended for messages which are not required + for research users but are needed when running operationally. + +- ``PrStatus_Diag`` or or ``PrDiag`` - The most verbose option, all messages + which do not fall into one of the above categories should use this setting. + Non-essential, detailed information about values of variables, status + messages, etc should be included in this category. If a developer adds code + to assist debugging problems, it should also be protected by + ``PrStatus_Diag``. + + +.. _`S14`: + +S14. DrHook +----------- + +DrHook is a library written by ECMWF which can produce run-time information +such as: + +- Per-routine profiling information based on walltime, CPU-time and MFlops. + +- Tracebacks in the event of code failure. A developer can force a traceback at + any point in the code with an appropriate call to the DrHook library. + +- Memory usage information. + +For DrHook to be effective, calls to the library are needed in each individual +subroutine. DrHook must be called: + +#. At the start of each routine, before any other executable code. + +#. At each exit point from the routine; not only at the end, but just before + any other ``RETURN`` statements. + +When adding DrHook to a routine, the following rules should be followed: + +- Routines contained in modules should include the name of the module in the + call to DrHook, colon-separated. E.g. ``'MODULE_NAME:ROUTINE_NAME'``. + +- All names should be in capitals. + +The necessary instrumentation code and the recommended method of implementing +it is shown below. + +.. code-block:: fortran + + CHARACTER(LEN=*), PARAMETER, PRIVATE :: ModuleName = 'MODULE_NAME' + + CONTAINS + ... + + USE parkind1, ONLY: jpim, jprb + USE yomhook, ONLY: lhook, dr_hook + + ... + CHARACTER(LEN=*), PARAMETER :: RoutineName = 'ROUTINE_NAME' + + INTEGER(KIND=jpim), PARAMETER :: zhook_in = 0 + INTEGER(KIND=jpim), PARAMETER :: zhook_out = 1 + REAL(KIND=jprb) :: zhook_handle + + IF (lhook) CALL dr_hook(ModuleName//':'//RoutineName,zhook_in,zhook_handle) + + ... + + IF (lhook) CALL dr_hook(ModuleName//':'//RoutineName,zhook_out,zhook_handle) + +The example subroutine shown in :ref:`example` demonstrates DrHook +instrumentation. + +Calls to DrHook add a very small overhead to the code, and so should normally +only be added to routines that do a non-trivial amount of work. Adding DrHook +calls to very small routines may represent a large increase in the workload of +those routines, and furthermore if those routines are called many thousands of +times during a single run of the UM then this will generate large amounts of +duplicate data. The developer and reviewer may decide it is unnecessary to +include DrHook calls in such routines. + +Note that there is no benefit to adding DrHook calls to a module that consists +only of Fortran declarations and lacks any executable code. + +DrHook calls should *not* be added to ``RECURSIVE`` routines as they are likely +to cause runtime errors. + + +.. _`S15`: + +S15. OpenMP +----------- + +OpenMP is a very powerful technology for introducing shared memory parallelism +to a code, but it does have some potential for confusion. To help minimise +this, the following should be adhered to, + +- Only use the OpenMP 3.1 standard. Support for OpenMP 4.0 is not yet + widespread, and implementations are somewhat immature. + +- Only use the ``!$OMP`` version of the directive and start at beginning of the + line (see previous general guidance on sentinels). + +- Never rely on the default behaviour for ``SHARED`` or ``PRIVATE`` variables. + The use of ``DEFAULT(NONE)`` is preferred, with the type of all variables + explicitly specified. A different ``DEFAULT`` may be allowed if the number + of variables is very large (i.e. dozens). + +- Parameters by default are shared. To make this obvious it is helpful to list + parameters used in the OMP block as a Fortran comment just before the + ``PARALLEL`` region. + +- Always use explicit ``!$OMP END DO`` - don't rely on implicit rules. + +- Unlike ``SINGLE`` regions, ``MASTER`` regions do not carry an implicit + barrier at the end. Please add an ``!$OMP BARRIER`` directive immediately + after ``!$OMP END MASTER`` directives. Barriers may be omitted for + performance reasons if it is safe to do so. + +- Calls to OpenMP functions and module use should be protected by the OpenMP + sentinel. That is, the line should start with ``!$`` and a space. No other + comment line should start with this combination. + +- Always specify the scheduler to be used for DO loops, since the default is + implementation specific. A common default is STATIC. This is normally fine + but can cause problems within certain cases. + +- As with non-OpenMP code, you should always use the optional space to separate + the OpenMP keywords to improve readability. For example, ``PARALLELDO`` + should become ``PARALLEL DO``. (See also: `S4`_) + +- Any use of a sentinel (including OpenMP) should start at the beginning of the + line, e.g. + + The following correctly uses the ``!$OMP`` sentinel at the beginning of the + line. + + .. code-block:: fortran + + IF (do_loop) THEN + !$OMP PARALLEL DO PRIVATE(i) + DO i = 1, 100 + ... + END DO + !$OMP PARALLEL DO + END IF + + Whilst the following can lead to compilers not using the lines starting with + ``!$OMP`` sentinel. + + .. code-block:: fortran + + IF (do_loop) THEN + !$OMP PARALLEL DO PRIVATE(i) + DO i = 1, 100 + ... + END DO + !$OMP PARALLEL DO + END IF + +- Careful use of the OpenMP reduction clauses is required as we want to try and + preserve bit-comparison across different threads. This is not guaranteed + with some ``REDUCTION`` clauses. + +- OpenMP directives in C code must be protected by both a + ``SHUM_USE_C_OPENMP_VIA_THREAD_UTILS`` and an ``_OPENMP`` if-def. This + ensures it is possible to select the use of only the Fortran OpenMP runtime + library, which can prevent incompatibilities between different libraries. If + possible, provide a Fortran implementation of the OpenMP parallelism as + well, using the wrappers in the ``thread_utils`` module from SHUMlib. + (Further rules apply; see :ref:`OpenMPinC` for more information.) + + +.. _`S16`: + +S16. MPI +-------- + +The Unified Model depends on the GCOM library for communications. GCOM has only +modest functionality however so the use of MPI is permitted providing the +following principles are adhered to: + +- Only use MPI via GCOM's MPL interface layer. MPI libraries can be found that + support only 32-bit argument or only 64-bit arguments. MPL is designed to + abstract this issue away. + +- Only use functionality from versions of MPI up to 3.1. These have widespread + support. + + +.. _`S17`: + +S17. Preprocessing +------------------ + +Use of preprocessor directives should only be used when its inclusion can be +justified, e.g. machine dependent options or reducing duplication of a large +code section, see `S18`_. + +Do not use preprocessing directives (``#if``, ``#include``, ``#endif``) for +selecting science code section versions. Do not use ``#include`` directive to +pass a large list of arrays or to pass common items. + +In particular: + +- "Must" use ``#if defined`` rather than ``#if``. If the CPP flag does not + exist the pre-processor evaluates the test to true. + +- Use run-time rather than compile time switches + +- Do not replicate run-time switches with compile-time ones, so avoid + + .. code-block:: fortran + + #if defined(OCEAN) + IF (submodel == ocean) THEN + #endif + ... + #if defined(OCEAN) + END IF + #endif + +- Do not add optional arguments to subroutines protected by directives, instead + migrate to FORTRAN 95/2003 code and make use of OPTIONAL argument + functionality. + +- Put ``#if`` lines inside included files rather than around the ``#include`` + itself. + +- Use directive names that clearly indicate their purpose. + +- When removing scientific sections, remove variables that were only needed for + that section. + +- Do not wrap a routine within CPP flags. Let the compiler work out when it is + required. + +- Please refrain from using consecutive question marks (``??``) in the source + code as some preprocessors can interpret them as C trigraphs. + + +.. _`S18`: + +S18. Code duplication +--------------------- + +In the case of a large area of code that needs to be duplicated, e.g. same +computation applied to different types, then the use of the ``#include`` +preprocessing directive is recommended by adhering the following rules: + +- Only one include file per routine. If a routine needs multiple include files, + consider dividing the routine into small multiple routines. The same include + file cannot be used in multiple modules or routines. Consider creating a + special routine with the shared code if needed. + +- Use ``*.h`` as a file extension for ``#include`` files since the build system + will automatically recognise it. + +- File name should always be ``modulename_routinename.h``. An accepted + exception is when the module name and the routine name are the same, e.g. + instead of ``routine_mod_routine.h`` use ``routine.h``. + +- The include file should be located in a special ``include`` sub-directory + where the Fortran module is located. + +- An include file should only be used for reducing code duplication, not for + performance reason. Let the compiler implement proper in-lining. + +The following code shows an example on how to use the ``#include`` +preprocessing directive inside a module to reduce code duplication. + +- The module file ``my_mod.F90`` in the ``src/path/to/mod`` directory with the + duplicated routines: + + .. code-block:: fortran + + INTERFACE calc_1 + MODULE PROCEDURE calc_1_32bit,calc_1_64bit + END INTERFACE + + SUBROUTINE calc_1_32bit(r,n,d) + IMPLICIT NONE + INTEGER, PARAMETER :: prec = real32 + + #include "my_mod_calc_1.h" + + END SUBROUTINE + + SUBROUTINE calc_1_64bit(r,n,d) + IMPLICIT NONE + INTEGER, PARAMETER :: prec = real64 + + #include "my_mod_calc_1.h" + + END SUBROUTINE + +- The included file ``my_mod_calc_1.h`` in the ``src/path/to/mod/include`` + directory with the shared code: + + .. code-block:: fortran + + ! --- Begin shared body of calc_1 --- + REAL(KIND=prec), INTENT(OUT) :: r + REAL(KIND=prec), INTENT(IN) :: n + REAL(KIND=prec), INTENT(IN) :: d + + r = n / d + ! --- End shared body of calc_1 --- + + +.. _`S19`: + +S19. Error reporting +-------------------- + +The most important rule in error reporting is *never* to ``CALL abort`` or to +use ``STOP``; these can cause problems in a parallel computing environment. +Where it is possible that errors may occur they should be detected and +appropriate action taken. Errors may be of two types: fatal errors requiring +program termination; and non-fatal warnings which allow the program to +continue. Both types are passed to a reporting routine ``ereport``, which +takes different actions depending on the value of the error code passed to it +as an argument: + +- If the error code is ``> 0`` an error message will be printed and the + program will abort (hopefully with a traceback). + +- If the error code is ``< 0`` a warning message will be printed, the error + code variable will be reset to 0, and the program continues. + +- If the error code is 0 nothing happens and the program continues + uninterrupted. + +Both warnings and errors are sent to the ``.pe``\ :math:`n` file *of the +processor generating the warning*, which is stdout for processor 0 only. +Warnings will only appear in stderr if they occur on processor 0. Errors will +always appear in stderr. Note that if a warning occurs on a processor for +which output has been disabled using the print manager settings, then that +warning will not be printed as there will be no ``.pe``\ :math:`n` file to +send it to. + +When using ``READ`` or ``OPEN`` or other Fortran intrinsics which deal with IO, +please use both the error status ``IOSTAT`` and the error message ``IOMSG`` +arguments, followed by code printing the latter if the former is non-zero. The +``check_iostat`` subroutine provides a convenient way to do this; any non-zero +value of ``IOSTAT`` will cause it to print the return value of ``IOMSG`` and +abort the program. + +- The arguments of ``ereport`` are: + + .. code-block:: fortran + + SUBROUTINE ereport (RoutineName, ErrorStatus,Message) + + CHARACTER(LEN=*), INTENT(IN) :: RoutineName ! Name of the calling routine + CHARACTER(LEN=*), INTENT(IN) :: Message ! Error message for output + INTEGER, INTENT(IN OUT) :: ErrorStatus ! Error code + +- Ensure the error code variable is set to zero before use. This includes at + the start of every routine where it is a local variable, and also before + calling any routine that returns it(``INTENT(IN OUT)``). + +- Error messages should contain enough information to *help* the user diagnose + and solve the problem. + +- Avoid splitting error information between stdout (``umprint``) and stderr + (``ereport``). Keep the details in one place where possible. If the nature + of the error requires large quantities of additional data in stdout to + diagnose it properly, make this clear in the error message. + +- The variable ``errormessagelength`` in module ``errormessagelength_mod`` is + provided for declaring the length of ``CHARACTER`` variables to be used with + error reporting. This provides a longer string for holding e.g. the return + value of an ``IMOSG`` argument. + +- Avoid using a namelist input value or the return code of another routine as + the error code, especially if you do not know what values it may take. It + may not be apparent to the user that the problem value is actually the error + code, or what sign it originally had. Use a dedicated error code and include + the return code or problematic value in the message itself. + + Common practice: + + .. code-block:: fortran + + IF (foo /= 0) THEN + icode = ABS(foo) + cmessage = 'Invalid input value for foo' + CALL ereport(RoutineName, icode, cmessage) + END IF + + Better approach: + + .. code-block:: fortran + + IF (foo /= 0) THEN + icode = 10 + WRITE(cmessage, '(A,I0)') 'Invalid input value for foo. Value received: ',foo + CALL ereport(RoutineName, icode, cmessage) + END IF + +.. _`sec:specific`: + +Specific standards +================== + +.. _namelists: + +Runtime namelist variables, defaults, future development +-------------------------------------------------------- + +The UM reads in a number of run time 'control' namelists; within READLSTA.F90. +Examples are the ``RUN_`` type namelists. When new science options +are required to be added to the UM the developer is expected to add the new +variable/parameter to the relevant ``RUN_`` namelist and declaration +in the corresponding module, updating READLSTA.F90 as required. + +The use of ``cruntimc.h`` is to be avoided as this approach is being phased out +in favour of suitable modules. + +- Code development should use MODULES to define namelist LOGICALS, PARAMETERS + and VARIABLES (and their defaults) alongwith the NAMELIST. + +- It is essential that defaults are set; items within namelists are expected to + fall into 3 camps: + + - variable never actually changes; it is a default for all users + + - this should be set in the code and removed from any input namelist. + + - variable rarely changes; + + - set identified default within UM code, with comment explaining choice. + + - We advise that these are not included in the namelist. A code change will + be required to alter it. + + - regularly changes or is a new item and thus no default is yet suitable + + - ``LOGICAL``\ s usually to ``FALSE`` + + - variables set to RMDI or IMDI + + - ``CHARACTER`` strings should be set to a default string. For + example, + + .. code-block:: fortran + + aero_data_dir = 'aero data dir is unset' + +An example of preferred practice see ``RUN_Stochastic``. The namelist variables +are all defined within a MODULE, ``stochastic_physics_run_mod.F90``, including +default values. + +Defensive input programming +--------------------------- + +When real or integer values are read into the code by a namelist, the Rose +metadata should either use a values list or a range so that the Rose GUI can +warn the user of invalid values. These values should also be tested in the +code to ensure that the values read in are valid. As it is possible to edit +Rose namelists, or ignore Rose GUI warnings, the GUI should not be relied on +for checking that input values of reals and integers are valid. It may also be +appropriate to check logical values if a specific combination of logicals will +cause an error for example. + +The routine, ``chk_var``, is available for developers to more easily check +their inputs. Checks made by ``chk_var`` should match any checks made by Rose, +however checks by ``chk_var`` are made by the code and will by default, abort +the run. Developers should refer to the `um-training +`__ for +more information on ``chk_var``. + +Optimised namelist reading procedures +------------------------------------- + +As of UM9.1 the procedure to read UM namelists has been enhanced but this has +implications for the code developer, requiring extra code changes when +adding/removing a UM input namelist item. Tied with each namelist read is now +the requirement for a 'read_nml_routine' usually found in the containing +module of the namelist. + +If a coder wishes to add a new variable to a namelist (xxxxxx) then the new +read_nml_xxxxxx subroutine will need changing. The changes required are: + +- increment the relevant type parameter by the variable size (for a real scalar + increase n_real by 1) + +- add a new line to the list in the my_namelist type declaration in the + relevant variable type. + +- add a new line to the my_nml population section in the relevant variable + type + +- add a new line to the namelist population section in the relevant variable + type. + +See the UM code for examples. + +Unix script standards +--------------------- + +This standard covers UM shell scripts which are used in the operational suite +as well as within the UM itself. The requirements that this standard is +intended to meet are as follows: + +- The script should be easily understood and used, and should be easy for a + programmer other than the original author to modify. + +- To simplify portability it should conform to the unix standard as much as + possible, and exclude obsolescent and implementation-specific features when + possible. + +- It should be written in an efficient way. + +- The structure of the script should conform to the design agreed in the + project plan. + +Scripts are to be regarded as being control code as far as external +documentation is concerned. + +Python standards +---------------- + +Python code used in or with the UM should obey the standard Python style guide +`PEP 8 `__. This means that our +Python code will follow the same guidelines commonly adhered to in other +Python projects, including Rose. + +C standards +----------- + +C code used in or with the UM should conform to the C99 standard +(`ISO/IEC 9899:1999: Programming languages - C (1999) by JTC 1/SC 22/WG 14 +`__). + +Furthermore, it is assumed that any C implementation used by the UM supports +C99 Annex F (IEC 60559 Floating-point arithmetic) i.e. it is assumed the +implementation defines ``__STDC_IEC_559__``. It is also assumed the +implementation provides the optional 8-, 16-, 32-, and 64-bit exact-width +integer types. + +Preprocessing of C +~~~~~~~~~~~~~~~~~~ + +Preprocessing of source files is allowed, as defined by the C99 standard, but +with a few minor exceptions. This use includes - but is not limited to - the +use of ``#include``, macros, ``#pragma``, and ``_Pragma`` statements. + +The exceptions are as follows: + +- Code must not be dependent on preprocessing to select optional or platform + specific features in order for it to compile or run. Platform specific and + optional code are allowed; but this should augment basic functionality + rather than implment a key component of it. In other words, code should be + able to compile and run correctly on all platforms without any optional or + platform dependent macros being defined, even if the code could take + advantage of them on that platform. + +- Platform specific code must be protected by an if-def test on a compiler + and/or platform specific macro as appropriate. (Examples may include the use + of ``__GNUC__``, ``__clang__``, ``__linux__``, ``_AIX``, ``__x86_64__``, or + ``__aarch64__``) This includes the protection of compiler-specific + ``#pragma``/``_Pragma`` statements. + +- If-def tests must not use the ``#ifdef``/``#ifndef`` style. Instead use ``#if + defined()`` or ``#if !defined()`` as appropriate. This restriction is + required to simplify the implementation of automated testing. + +Code Layout +~~~~~~~~~~~ + +Rules regarding whitespace, 80 column line widths, prohibition on tab use, and +the use of UK English apply to C code as they would Fortran code. Comments +should use the traditional ``/* */`` style; C++ style comments (``//``) should +be avoided. + +Copyright and Code Owner Comments +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Copyright and code owner comments follow the same rules as in Fortran, except +with slight modification for the differing comment delimiters in the two +languages — using ``/* */`` instead of ``!``. An example of a compliant +comment header detailing copyright and code owner comments is given below. + +.. container:: minipage + + .. code-block:: C + + + /**********************************COPYRIGHT***********************************/ + /* (C) Crown copyright Met Office. All rights reserved. */ + /* For further details please refer to the file COPYRIGHT.txt */ + /* which you should have received as part of this distribution. */ + /**********************************COPYRIGHT***********************************/ + + /* Code Owner: Please refer to the UM file CodeOwners.txt */ + /* This file belongs in section: C Code */ + +Deprecated identifiers +~~~~~~~~~~~~~~~~~~~~~~ + +In addition to the identifiers deprecated by the C99 standard, the following +table lists identifiers which should be considered deprecated within UM code — +and where appropriate, what to replace them with. + +========================== ===================== +**Deprecated indentifier** **Replace with** +========================== ===================== +``sprintf()`` ``snprintf()`` [#f1]_ +``strcpy()`` ``strncpy()`` [#f1]_ +========================== ===================== + +.. [#f1] These functions take different arguments from the original deprecated functions they replace. + + +.. _OpenMPinC: + +OpenMP in C Code +~~~~~~~~~~~~~~~~ + +It is possible for the runtime libraries used by OpenMP to be incompatible if +different vendors or compiler versions are used for the C and Fortran +compiler. For this reason, whilst use of OpenMP in C code is permitted, there +are some rules governing acceptable use that must be followed. + +Protecting OpenMP in C Code +~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +OpenMP directives (``#pragma omp``) in C code must be protected by both a +``SHUM_USE_C_OPENMP_VIA_THREAD_UTILS`` and an ``_OPENMP`` ``#ifdef``. This +ensures it is possible to select the use of only the Fortan OpenMP runtime +library if required. If possible, provide a Fortran implementation of the +OpenMP parallelism as well, using the wrappers in the ``thread_utils`` module +from SHUMlib. An example of such use is given below. + +.. container:: minipage + + .. code-block:: C + + #if defined(_OPENMP) && defined(SHUM_USE_C_OPENMP_VIA_THREAD_UTILS) + + /* this branch uses the Fortran OpenMP runtime, via the SHUMlib thread_utils module */ + thread_utils_func(); + + #elif defined(_OPENMP) && !defined(SHUM_USE_C_OPENMP_VIA_THREAD_UTILS) + + /* this branch uses OpenMP pragmas within C */ + #pragma omp parallel + { + omp_func(); + } + + #else + + /* this branch does not use OpenMP */ + serial_func(); + + #endif + +Ideally this should lead to code capable of providing all three possible +runtime outcomes, the use of which are compile-time configurable: + +- No OpenMP is used. + +- OpenMP is used through the C runtime library. (The compiler defines + ``_OPENMP``, through the nomal compiler switch selection process.) + +- OpenMP is used through the Fortran runtime library, accesed via SHUMlib. + (The compiler defines ``_OPENMP``; the user defines + ``SHUM_USE_C_OPENMP_VIA_THREAD_UTILS``) + +You must always ensure that the no OpenMP case is possible. + +(See also: The SHUMlib documentation on ``shum_thread_utils``) + +Other Uses of the \_OPENMP Macro +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +The use of the ``_OPENMP`` preprocessor macro for code other than directives is +permitted. This can be used equivalently to how the ``!$`` sentinel would be +in Fortran. A recommended use is to protect the inclusion of the header for +the ``thread_utils`` module, as shown below. + +.. container:: minipage + + .. code-block:: C + + + #if defined(_OPENMP) && defined(SHUM_USE_C_OPENMP_VIA_THREAD_UTILS) + #include "c_shum_thread_utils.h" + #endif + +Or to protect inclusion of the OpenMP header, as shown below. + +.. container:: minipage + + .. code-block:: C + + + #if defined(_OPENMP) && !defined(SHUM_USE_C_OPENMP_VIA_THREAD_UTILS) + #include + #endif + +Further Rules for OpenMP in C +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +In order to standardise the way the above rules are implemented, and to allow +for automated checking of the compliance of code, the following additional +rules are imposed. + +- You cannot hide the use of the ``_OPENMP`` & + ``SHUM_USE_C_OPENMP_VIA_THREAD_UTILS`` macros through the definition of a + third macro dependent on them. For example, you must not define and use a + new macro in place of the two original macros, as shown here: + + .. container:: minipage + + .. code-block:: C + + + #define USE_THREAD_UTILS defined(_OPENMP) && defined(SHUM_USE_C_OPENMP_VIA_THREAD_UTILS) + + #if defined(USE_THREAD_UTILS) + thread_utils_func(); + #endif + +- If-def tests on ``_OPENMP`` & ``SHUM_USE_C_OPENMP_VIA_THREAD_UTILS`` must + always occur as a pair. You may not test the use of ``_OPENMP`` or + ``SHUM_USE_C_OPENMP_VIA_THREAD_UTILS`` in isolation. + +- ``_OPENMP`` must come first in any ``#if defined()`` pair. + +- Any OpenMP ``#if defined()`` pair must not also include a logical test on a + third macro. If this functionality is required, find an appropriate nesting + of ``#if defined()`` tests. For example instead of: + + .. container:: minipage + + .. code-block:: C + + + #if defined(_OPENMP) && defined(SHUM_USE_C_OPENMP_VIA_THREAD_UTILS) && defined(OTHER) + /* do stuff */ + #endif + + Use: + + .. container:: minipage + + .. code-block:: C + + #if defined(_OPENMP) && defined(SHUM_USE_C_OPENMP_VIA_THREAD_UTILS) + #if defined(OTHER) + /* do stuff */ + #endif + #endif + +- You must not use negative logic in an if-def test on ``_OPENMP`` + (i.e. ``#if !defined(_OPENMP)``). Instead, use positive logic and an + ``#else`` branch. Use of negative logic is permitted for if-def tests on the + accompanying ``SHUM_USE_C_OPENMP_VIA_THREAD_UTILS`` macro, as this will be + required to distinguish between cases using the C and Fortan OpenMP + runtimes. + +.. _`sec:reviews`: + +Code Reviews +============ + +In order to ensure that these standards are adhered to and are having the +desired effect code reviews must be held. Reviews can also be useful in +disseminating computing skills. To this end two types of code review are +performed in the order below: + +#. A science/technical review is performed first to ensure that the code + performs as it is intended, it complies with the standards and is well + documented. Guidance for reviewers is found in the `Science/Technical + Review Guidance + `__ + page on the UM homepage. + +#. A Code Review is performed to analyse the change for its impact, + ensure that it meets this coding standard and to ensure that all concerned + parties are made aware of changes that are required. Guidance for reviewers + is outlined in `Code Review Guidance page + `__. + + +.. _Appendix A: + +A. UM Software standard summary +=============================== + +The rules discussed in the main text are reproduced here in summary form with +pdf links to the sections. + ++----------------------------------+----------------------------------+ +| **Standard** | **Section** | ++==================================+==================================+ +| Use the naming convention for | `S1`_ | +| program units. | | ++----------------------------------+----------------------------------+ +| Use your header and supply the | `S2`_ | +| appropriately complete code | | +| header | | ++----------------------------------+----------------------------------+ +| History comments are NOT | `S2`_ | +| required and should be removed | | +| from routines. | | ++----------------------------------+----------------------------------+ +| Fortan code should be written in | `S3`_ | +| free source form | | ++----------------------------------+----------------------------------+ +| Code must occur in columns 1-80 | `S3`_ | +| (1-100 for CreateBC). | | ++----------------------------------+----------------------------------+ +| Never put more than one | `S3`_ | +| statement per line. | | ++----------------------------------+----------------------------------+ +| Use English in your code. | `S3`_ | ++----------------------------------+----------------------------------+ +| All Fortran keywords should be | `S4`_ | +| ALL CAPS while everything else | | +| is lowercase or CamelCase. | | ++----------------------------------+----------------------------------+ +| Avoid archaic Fortran features | `S4`_ | ++----------------------------------+----------------------------------+ +| Only use the generic names of | `S4`_ | +| intrinsic functions | | ++----------------------------------+----------------------------------+ +| Comments start with a single | `S5`_ | +| ``!`` at beginning of line. | | ++----------------------------------+----------------------------------+ +| Single line comments can be | `S5`_ | +| indented within the code, after | | +| the statement. | | ++----------------------------------+----------------------------------+ +| Do not leave a blank line after | `S5`_ | +| a comment line. | | ++----------------------------------+----------------------------------+ +| Do NOT use TABS within UM code. | `S5`_ | ++----------------------------------+----------------------------------+ +| The use of MODULEs is greatly | `S6`_ | +| encouraged. | | ++----------------------------------+----------------------------------+ +| Use meaningful variable names | `S7`_ | ++----------------------------------+----------------------------------+ +| Use and declare variables and | `S7`_ | +| arguments in the order | | ++----------------------------------+----------------------------------+ +| Use ``INTENT`` in declaring | `S7`_ | +| arguments | | ++----------------------------------+----------------------------------+ +| Use ``IMPLICIT NONE``. | `S7`_ | ++----------------------------------+----------------------------------+ +| Use ``REAL, EXTERNAL :: func1`` | `S7`_ | +| for functions | | ++----------------------------------+----------------------------------+ +| Do not use ``EXTERNAL`` | `S7`_ | +| statements for subroutines | | ++----------------------------------+----------------------------------+ +| The use of ALLOCATABLE arrays | `S8`_ | +| can optmize memory use. | | ++----------------------------------+----------------------------------+ +| Indent code within ``DO`` or | `S9`_ | +| ``IF`` blocks by 2 characters | | ++----------------------------------+----------------------------------+ +| Terminate loops with ``END DO`` | `S9`_ | ++----------------------------------+----------------------------------+ +| ``EXIT`` statements must be | `S9`_ | +| labelled | | ++----------------------------------+----------------------------------+ +| Avoid comparing two reals | `S9`_ | +| ``IF ( real1 == real2 ) THEN`` | | ++----------------------------------+----------------------------------+ +| Avoid using 'magic numbers' and | `S9`_ | +| 'magic logicals' | | ++----------------------------------+----------------------------------+ +| Avoid use of ``GO TO`` | `S9`_ | ++----------------------------------+----------------------------------+ +| Avoid numeric labels | `S9`_ | +| | `S12`_ | ++----------------------------------+----------------------------------+ +| Exception is for error trapping, | `S9`_ | +| jump to the label ``9999`` | | +| ``CONTINUE`` statement. | | ++----------------------------------+----------------------------------+ +| Continuation line marker must be | `S10`_ | +| ``&`` at the end of the line. | | ++----------------------------------+----------------------------------+ +| Always use an ``ACTION`` when | `S11`_ | +| you ``OPEN`` a file. | | ++----------------------------------+----------------------------------+ +| Check for file existence with | `S11`_ | +| ``OPEN`` rather than ``INQUIRE`` | | ++----------------------------------+----------------------------------+ +| Always format information | `S12`_ | +| explcitly within WRITE, READs | | +| etc. | | ++----------------------------------+----------------------------------+ +| Ensure that output messages do | `S12`_ | +| not use | | +| ``WR | | +| ITE(6,...)``,\ ``WRITE(*,...)``, | | +| or ``PRINT*``. | | ++----------------------------------+----------------------------------+ +| Ensure that output messages are | `S13`_ | +| protected by an appropriate | | +| setting of ``PrintStatus``. | | ++----------------------------------+----------------------------------+ +| Ensure your subroutines are | `S14`_ | +| instrumented for DrHook. | | ++----------------------------------+----------------------------------+ +| Only use OpenMP sentinels at the | `S15`_ | +| beginning of lines ``!$OMP`` | | ++----------------------------------+----------------------------------+ +| Be very careful when altering | `S15`_ | +| calculations within a OpenMP | | +| block. | | ++----------------------------------+----------------------------------+ +| If possible implement runtime | `S17`_ | +| logicals rather than compile | | +| time logicals. | | ++----------------------------------+----------------------------------+ +| Do not replicate (duplicate) | `S17`_ | +| runtime logic with cpp logic. | | ++----------------------------------+----------------------------------+ +| Do not protect optional | `S17`_ | +| arguments with cpp flags, use | | +| OPTIONAL args instead. | | ++----------------------------------+----------------------------------+ +| Do not use CPP flags for | `S17`_ | +| selecting science code, use | | +| runtime logicals | | ++----------------------------------+----------------------------------+ +| Use | `S18`_ | +| ``#incl | | +| ude "modulename_routinename.h"`` | | +| preprocessing directive for | | +| reducing code duplication | | ++----------------------------------+----------------------------------+ +| Never use ``STOP`` and | `S19`_ | +| ``CALL abort`` | | ++----------------------------------+----------------------------------+ +| New namelist items should begin | `namelists`_ | +| life as category c items. | | ++----------------------------------+----------------------------------+ + + + +.. _Appendix B: + +B. Fortran 2003 +=============== + +The following table provides guidance on which Fortran 2003 features are +welcome for inclusion in the UM. + +This has been compiled upon review of `major Fortran compilers +`__ feature support. + ++-------------------------+----------------+-------------------------+ +| **Feature** | **Acceptable** | **Comment** | ++=========================+================+=========================+ +| ISO TR 15581 | Yes | | +| Allocatable | | | +| Enhancements | | | ++-------------------------+----------------+-------------------------+ +| Interoperability with C | Yes | | ++-------------------------+----------------+-------------------------+ +| Access to the computing | Yes | | +| environment | | | ++-------------------------+----------------+-------------------------+ +| Flush | Yes | | ++-------------------------+----------------+-------------------------+ +| IOMSG | Yes | | ++-------------------------+----------------+-------------------------+ +| Assignment to an | No | Includes | +| allocatable array | | auto-reallocation | ++-------------------------+----------------+-------------------------+ +| Intrinsic Modules | Yes | eg ISO_C_BINDING | ++-------------------------+----------------+-------------------------+ +| Allocatable Scalars | Yes | | ++-------------------------+----------------+-------------------------+ +| Allocatable Character | Yes | gnu offers partial | +| lengths | | support. | ++-------------------------+----------------+-------------------------+ +| VOLATILE attribute | Yes | | ++-------------------------+----------------+-------------------------+ +| Parametrized derived | No | Lack of compiler | +| data types | | support | ++-------------------------+----------------+-------------------------+ +| O-O coding: type | No | Not for the current UM, | +| extension, polymorphic | | but considered for the | +| entities, type bound | | UM replacement, | +| procedures | | LFRIC-GUNGHO and MakeBC | +| | | replacement CreateBC | ++-------------------------+----------------+-------------------------+ +| Derived type input | No | Lack of compiler | +| output | | support | ++-------------------------+----------------+-------------------------+ +| Kind type parameters of | No | Lack of compiler | +| integer specifiers | | support | ++-------------------------+----------------+-------------------------+ +| Recursive input/output | No | | ++-------------------------+----------------+-------------------------+ +| Transferring an | No | Prefer to see | +| allocation | | DEALLOCATEs used for | +| | | code readability. | ++-------------------------+----------------+-------------------------+ +| Support for | No | | +| international character | | | +| sets | | | ++-------------------------+----------------+-------------------------+ + + +.. _Appendix C: + +C. Dealing with rounding issues. +================================ + +Background +---------- + +The UM perturbation sensitivity project identified coding issues that lead to +excessive perturbation growth in the model. Problems identified included +``IF`` tests that contained comparisons between real numbers; for example +``IF (qCL(i) > 0.0 )`` In this test, ``qCL(i)`` is being used to represent one +of two states; + +- "no liquid cloud" + +- "some liquid cloud" + +This is fine, but it is then important to ensure that rounding issues do not +lead to unintended changes of state prior to the test, such as slightly +non-zero ``qCL(i)`` values when there is supposed to be no liquid cloud. If +such problems occur at discontinuous branches in the code, the result is +spurious perturbation growth. + +This appendix collects together some typical examples of what can go wrong, and +how to deal with them. First, though, it is worth making a quick note of some +of the characteristics of floating-point arithmetic. + +Floating-point identities and non-identities +-------------------------------------------- + +In floating-point arithmetic many of the identities that hold in normal +arithmetic no longer hold, basically because of the limited precision +available to represent real numbers. Thus, it is often important that coders +know which algebraic identities pass through to floating-point arithmetic and +which don't, and how results can be affected by the way the calculations are +implemented by the compiler. For chapter and verse on floating-point +arithmetic, a good reference is "`David Goldberg's article +`__ " + +The following floating-point identity always holds: + +:: + + 0.0 * x = 0.0 + +The following also hold, but only if the numbers that go into the calculations +have the same precision: + +:: + + 0.0 + x = x + 1.0 * x = x + x / x = 1.0 + x - x = 0.0 + x - y = x + (-y) + x + y = y + x + x * y = y * x + 2.0 * x = x + x + 0.5 * x = x / 2.0 + +For example, optimisation may lead to some variables being held in cache and +others in main memory, and these will generally store numbers with different +levels of precision. Thus, coding based on these identities will probably work +as intended in most circumstances, but may be vulnerable to higher levels of +optimisation. + +The following are non-identities: + +:: + + x + (y + z) /= (x + y) + z + x * (y * z) /= (x * y) * z + x * (y / z) /= (x * y) / z + +These say that, unlike in normal arithmetic, the order of the calculations +matters. Failure to recognise this can cause problems, as in example 1 below. +(Note that putting brackets around calculations to try and impose +the "correct" order of calculation will not necessarily work; the compiler +will decide for itself!) + +Example 1: Non-distributive arithmetic +-------------------------------------- + +At UM vn7.4, the routine ``LSP_DEPOSITION`` contains the following +calculation: + +.. code-block:: fortran + + ! Deposition removes some liquid water content + ! First estimate of the liquid water removed is explicit + dqil(i) = max (min ( dqi_dep(i)*area_mix(i) & + & /(area_mix(i)+area_ice_1(i)), & + & qcl(i)*area_mix(i)/cfliq(i)) ,0.0) + ... + If (l_seq) Then + qcl(i) = qcl(i) - dqil(i) ! Bergeron Findeisen acts first + +Here, ``dqil`` is a change to cloud liquid water ``qcl``, which is limited in +the calculation to ``qcl*area_mix/cfliq``, where ``area_mix`` is the fraction +of the gridbox with both liquid and ice cloud, and ``cfliq`` is the fraction +with liquid cloud. Basically, the change to cloud liquid water is being +limited by the amount of liquid cloud which overlaps with ice cloud it can +deposit onto. + +In the special case that all the liquid cloud coincides with ice cloud, we have +``area_mix = cfliq``, implying ``area_mix/cfliq = 1.0``. In this case, the +limit for ``dqil`` should be exactly ``qcl``, but is coded as +``qcl*area_mix/cfliq``. In tests on the IBM, it seems that the compiler +decides that the multiplication should precede the division, so the outcome of +the calculation is not necessarily ``qcl``. Thus, the update to ``qcl`` on the +last line does not necessarily lead to ``qcl = 0.0`` when the limit is hit. + +One solution to this problem is to supply ``area_mix/cfliq`` directly as a +ratio: + +.. code-block:: fortran + + If (cfliq(i) /= 0.0) Then + areamix_over_cfliq(i)=area_mix(i)/cfliq(i) + End if + ... + ! Deposition removes some liquid water content + ! First estimate of the liquid water removed is explicit + dqil(i) = max (min ( dqi_dep(i)*area_mix(i) & + & /(area_mix(i)+area_ice_1(i)), & + & qcl(i)*areamix_over_cfliq(i)) ,0.0) + +This is the solution we have adopted in the large-scale precipitation code. + +Example 2: Changing units when applying limits +---------------------------------------------- + +At UM vn7.4, the routine ``LSP_TIDY`` contains the following calculation: + +.. code-block:: fortran + + ! Calculate transfer rate + dpr(i) = temp7(i) / lfrcp ! Rate based on Tw excess + + ! Limit to the amount of snow available + dpr(i) = min(dpr(i) , snow_agg(i) & + & * dhi(i)*iterations*rhor(i) ) + ... + ! Update values of snow and rain + If (l_seq) Then + snow_agg(i) = snow_agg(i) - dpr(i)*rho(i)*dhilsiterr(i) + qrain(i) = qrain(i) + dpr(i) + +where + +.. code-block:: fortran + + dhilsiterr(i) = 1.0/(dhi(i)*iterations) + rhor(i) = 1.0/rho(i) + +Here, ``dpr`` is a conversion rate from snow into rain, and the second +statement limits this rate to that required to melt all of the snow within the +timestep. Thus, the intention is that if this limit is hit the final snow +amount will come out to exactly 0.0. However, the outcome in this case is +effectively as follows: + +.. code-block:: fortran + + dpr(i) = snow_agg(i) * dhi(i)*iterations*rhor(i) + snow_agg(i) = snow_agg(i) - dpr(i)*rho(i)*dhilsiterr(i) + ( = snow_agg(i) & + - snow_agg(i) & + * dhi(i)*iterations*rhor(i)*rho(i)*1.0/(dhi(i)*iterations) ) + +In normal arithmetic, the multiplier on the final line comes out to exactly +one, but this is not necessarily the case in floating-point arithmetic. +Whether the expression comes out to exactly 1.0 or not will be highly +sensitive to the values going into the calculation. If the result is slightly +different to 1.0, the outcome is likely to be a tiny but non-zero snow +amount. + +The basic problem here is that the limit comes from a particular quantity, but +is being applied indirectly via its rate of change. Thus when the limiting +quantity is updated a change of units is required. The solution here is to +apply the limit to the quantity itself, shifting the change of units to +calculations involving rates: + +.. code-block:: fortran + + ! Calculate transfer + dp(i) = rho(i)*dhilsiterr(i)*temp7(i) / lfrcp + + ! Limit to the amount of snow available + dp(i) = min(dp(i), snow_agg(i)) + ... + ! Update values of snow and rain + If (l_seq) Then + snow_agg(i) = snow_agg(i) - dp(i) + qrain(i) = qrain(i) + dp(i)*dhi(i)*iterations*rhor(i) + +Example 3: Dealing with special cases +------------------------------------- + +At UM vn7.4, the routine ``LS_CLD`` contains the following calculation to +update the total cloud fraction ``CF`` given the liquid and frozen cloud +fractions ``CFL`` and ``CFF``: + +.. code-block:: fortran + + TEMP0=OVERLAP_RANDOM + TEMP1=0.5*(OVERLAP_MAX-OVERLAP_MIN) + TEMP2=0.5*(OVERLAP_MAX+OVERLAP_MIN)-OVERLAP_RANDOM + CF(I,J,K)=CFL(I,J,K)+CFF(I,J,K) & + & -(TEMP0+TEMP1*OVERLAP_ICE_LIQUID & + & +TEMP2*OVERLAP_ICE_LIQUID*OVERLAP_ICE_LIQUID) + ! Check that the overlap wasnt negative + CF(I,J,K)=MIN(CF(I,J,K),CFL(I,J,K)+CFF(I,J,K)) + +During testing, it was observed that ``CF`` was often coming out to +``0.9999999999999....``; i.e., almost but not quite 1.0, and that whether this +occured was highly sensitive to the input data. This sensitivity was then +being passed down to branches testing on, for example, whether ``CF`` was +equal to ``CFF``. + +If the above calculations are followed through algebraically, it can be shown +that if ``CFL+CFF >= 1``, then ``CF`` must be exactly one. In the +floating-point case, however, this no longer follows, so we often get cases +where there is a slight deviation from unity. The simplest solution in this +example is to deal with the special case separately: + +.. code-block:: fortran + + TEMP0=OVERLAP_RANDOM + TEMP1=0.5*(OVERLAP_MAX-OVERLAP_MIN) + TEMP2=0.5*(OVERLAP_MAX+OVERLAP_MIN)-OVERLAP_RANDOM + ! CFF + CFL >= 1 implies CF = 1 + IF (CFL(I,J,K)+CFF(I,J,K) >= 1.0) THEN + CF(I,J,K) = 1.0 + ELSE + CF(I,J,K)=CFL(I,J,K)+CFF(I,J,K) & + & -(TEMP0+TEMP1*OVERLAP_ICE_LIQUID & + & +TEMP2*OVERLAP_ICE_LIQUID*OVERLAP_ICE_LIQUID) + ! Check that the overlap wasnt negative + CF(I,J,K)=MIN(CF(I,J,K),CFL(I,J,K)+CFF(I,J,K)) + END IF diff --git a/source/FurtherDetails/contributing.rst b/source/FurtherDetails/contributing.rst new file mode 100644 index 00000000..ea4ab022 --- /dev/null +++ b/source/FurtherDetails/contributing.rst @@ -0,0 +1,42 @@ +.. ----------------------------------------------------------------------------- + (c) Crown copyright Met Office. All rights reserved. + The file LICENCE, distributed with this code, contains details of the terms + under which the code may be used. + ----------------------------------------------------------------------------- + +.. _contributing: + +Contributor Licence Agreement and Certificate of Origin +======================================================== + + +To best ensure that the BSD licence of Momentum code can be sustained, we +request that contributors from outside the Met Office agree with the following +terms when submitting code for inclusion in Momentum. + +By making a contribution to this project, I certify that: + +1. The contribution was created in whole or in part by me and I have the right + to submit it, either on my behalf or on behalf of my employer, under the + terms and conditions as described by this file; or +2. The contribution is based upon previous work that, to the best of my + knowledge, is covered under an appropriate licence and I have the right or + permission from the copyright owner under that licence to submit that work + with modifications, whether created in whole or in part by me, under the + terms and conditions as described by this file; or +3. The contribution was provided directly to me by some other person who + certified (1) or (2) and I have not modified it. +4. I understand and agree that this project and the contribution are public + and that a record of the contribution (including my name and email address) + is retained for the full term of the copyright and may be redistributed + consistent with this project or the licence(s) involved. +5. I, or my employer, grant to the UK Met Office and all recipients of this + software a perpetual, worldwide, non-exclusive, no-charge, royalty-free, + irrevocable copyright licence to reproduce, modify, prepare derivative + works of, publicly display, publicly perform, sub-licence, and distribute + this contribution and such modifications and derivative works consistent + with this project or the licence(s) involved or other appropriate open + source licence(s) specified by the project and approved by the + [Open Source Initiative (OSI)](http://www.opensource.org/). +6. If I become aware of anything that would make any of the above inaccurate, + in any way, I will let the UK Met Office know as soon as I become aware. diff --git a/source/FurtherDetails/dos_donts.rst b/source/FurtherDetails/dos_donts.rst index bded8e5b..e8457de1 100644 --- a/source/FurtherDetails/dos_donts.rst +++ b/source/FurtherDetails/dos_donts.rst @@ -1,53 +1,55 @@ +.. ----------------------------------------------------------------------------- + (c) Crown copyright Met Office. All rights reserved. + The file LICENCE, distributed with this code, contains details of the terms + under which the code may be used. + ----------------------------------------------------------------------------- + .. _dos_donts: Do's and Don'ts =============== + Please Do --------- + **Consult** Code Owners and system team. This will help maintain awareness and mitigate problems early on. This is the most common root cause of problems, sometimes years later. -**Plan** your work aimed at the trunk across single or multiple tickets: - * Ensure tickets are not too big or small. - * Coherent parts of the overall change are contained in a single ticket - * Consider using an overarching ticket to link everything together +**Plan** your work aimed across single or multiple Issues and Pull Requests: -**Document your work** using tickets, TRAC pages, :ref:`formal documentation ` -and code comments. These help others and your future-self understand your work. + * Each pull request should contain a single coherent change. + * Consider using an overarching issue to link everything together -**Meaningful names** for tickets, branches and variables. These help others and -your future-self understand your work. "My_Branch", "Fix" are not helpful. +**Document your work** using Issues and pull requests, :ref:`formal documentation +` and code comments. These help others and your future-self understand +your work. -**Be considerate** of other users/developers. Their skill-sets and working days may be very different to yours. All changes are visible to all users worldwide. +**Meaningful names** for issues, pull requests, branches and variables. These +help others and your future-self understand your work. "My_Branch", "Fix" are +not helpful. -**Keep the ticket status up to date.** This enables the Simulation Systems -and Deployment Team to monitor the progress of your ticket and potential conflicts. +**Be considerate** of other users/developers. Their skill-sets and working days +may be very different to yours. All changes are visible to all users +worldwide. -**Link to tickets in other MOSRS repositories**, eg jules:#1, ukca:#72 +**Link to issues/pull requests in other repositories**, eg ``MetOffice/jules#1``, +``MetOffice/ukca:#72`` Please Do Not ------------- -**Do not use svn commands.** Please use `FCM `_ for all development work. +**Do not merge ``main`` into your branch** during the development process. To +aid scientific evaluation, changes should be kept in standalone branches based +on the last stable version. -**Do not merge the trunk into your branch** for UM, JULES, UKCA and LFRic Apps changes as this breaks many aspects of how -TRAC and fcm work. This will cause diffs to display incorrectly and causes -database problems when merging. Instead, please create a head of trunk branch -and merge in your old branch. - -**Do not develop using head of trunk branching if not needed.** Many aspects of -the UM, JULES and UKCA workflows rely on version branching. +When your change is ready for Code Review, we then suggest merging in ``main`` +and resolving any conflicts. **Licensing** - Don't add code to any project (or to any branch thereof) that has been developed under a different license without agreement from the Simulation Systems and Deployment Team. This includes lifting Fortran code or text from books. Our repositiories must not infringe copyright. -**Add or link to old code** or tickets that predate MOSRS, for example... - -* Link to tickets in old internal repositories- links will either not resolve or be incorrect -* Add a version of the UM code older than UM 9.2 as a branch to the UM repository - -**Request support by raising a ticket**. Newly raised tickets are not monitored. -Use the appropriate :ref:`support` channels. \ No newline at end of file +**Request support by raising an issue**. Newly raised issues are not +monitored. Use the appropriate :ref:`support` channels. diff --git a/source/FurtherDetails/glossary.rst b/source/FurtherDetails/glossary.rst index 5fccee2c..31313051 100644 --- a/source/FurtherDetails/glossary.rst +++ b/source/FurtherDetails/glossary.rst @@ -1,64 +1,56 @@ +.. ----------------------------------------------------------------------------- + (c) Crown copyright Met Office. All rights reserved. + The file LICENCE, distributed with this code, contains details of the terms + under which the code may be used. + ----------------------------------------------------------------------------- + Simulation Systems Glossary =========================== + Please suggest new entries to the Simulation Systems and Deployment Team Closed Release: - A release cycle where the only accepted changes to the trunk relate to a + A release cycle where the only accepted changes to ``main`` relate to a particular piece of work, either technical or scientific. A release can also be partially closed, with only one area of a code base locked down in this way, and the rest free for changes. Code Review Deadline: - The date by which all tickets aiming to be included in a release have been + The date by which all pull requests aiming to be included in a release have been moved into code review. -Colon Keyword: - The formatting pattern for certain ticket keywords. For example CR:user to - indicate that "user" will be performing the Code System Review. - -CodeSys Review: - A technical review of the changes involved in the ticket, including checks - that code standards have been upheld and that the working practices have - been followed. These reviews are generally completed by a member of - the Simulation Systems and Deployment Team. Once a review has been approved - the Code Systems Reviewer is then responsible for committing the change to - the trunk. - -.. - or the Core Capability Development Team (for LFRic only reviews). - - +Code Review: + A technical review of the changes involved in the pull request, including + checks that code standards have been upheld and that the working practices + have been followed. These reviews are generally completed by a member of the + Simulation IT Team. Once a review has been approved the Code Systems + Reviewer is then responsible for committing the change to the ``main``. Development Window: - The period of time between the release of one software version and the - code review deadline for the following release in which new developments are + The period of time between the release of one software version and the code + review deadline for the following release in which new developments are accepted for review. -Further Commit: - Where a problem is found with a ticket after it has been committed, any - additional commits needed are associated with the same ticket and labelled - as a "Further Commit". Only essential and immediate fixes are treated this - way. - Known Good Output (KGO): - In order to verify that the model output hasn't been modified by a set of changes - the test suite contains a stored set of output as a reference. This is known - as the KGO and changes that alter this require special treatment. For more - information see :ref:`kgo`. + In order to verify that the model output hasn't been modified by a set of + changes the test suite contains a stored set of output as a reference. + This is known as the KGO and changes that alter this require special + treatment. For more information see :ref:`kgo`. -Head of Trunk: - The most recent code revision on the trunk. Branches are taken from here +Head of ``Main``: + The most recent commit hash on ``main``. Branches are taken from here when the work being done *has* to be built on top of changes already made - since the last revision. + since the last release. -Linked Ticket: - Work that spans two or more repositories, requiring tickets that should be - treated together and committed as a group. +Linked Pull Request (Linked PR): + Work that spans two or more repositories, requiring pull requests that + should be treated together and committed as a group. -Overarching Ticket: - Where a piece of work has been split into multiple sections and tickets an - extra ticket can be used to track this work. It should be closed when the - whole arc has been completed. +Overarching Issue: + Where a piece of work has been split into multiple sections an overarching + issue can be used to track this work. It should be closed when the whole arc + has been completed. GitHub also allows subissues which can be created from + the overarching issue. Regression: A set of tests that prove that a set of code changes have not degraded the @@ -72,15 +64,15 @@ SciTech Review: appropriate. Simulation Systems and Deployment Team: - The team responsible for the oversight of these working practices. For - more information see :ref:`ssd`. + The team responsible for the oversight of these working practices. For more + information see :ref:`simIT`. Simulation Systems Governance Group: The governing body that oversees the work of the Simulation Systems and Deployment Team. Version: - Each release of the codebase is completed by tagging the latest revision of - the trunk with a version number. This version should be used for creating - code branches from and will also be used by the parallel suite teams as a - starting point for creating the next operational suite. \ No newline at end of file + Each release of the codebase is completed by tagging the ``stable`` branch + with a version number. This version should be used for creating code + branches from and will also be used by the parallel suite teams as a + starting point for creating the next operational suite. diff --git a/source/FurtherDetails/index.rst b/source/FurtherDetails/index.rst index b0b68305..83d2c37b 100644 --- a/source/FurtherDetails/index.rst +++ b/source/FurtherDetails/index.rst @@ -1,3 +1,9 @@ +.. ----------------------------------------------------------------------------- + (c) Crown copyright Met Office. All rights reserved. + The file LICENCE, distributed with this code, contains details of the terms + under which the code may be used. + ----------------------------------------------------------------------------- + .. _further_details: Further Details @@ -11,5 +17,8 @@ Further Details support glossary code_of_conduct + contributing + ai dos_donts - change_notes \ No newline at end of file + change_notes + coding_style diff --git a/source/FurtherDetails/support.rst b/source/FurtherDetails/support.rst index f9ead184..9e55d07c 100644 --- a/source/FurtherDetails/support.rst +++ b/source/FurtherDetails/support.rst @@ -1,3 +1,9 @@ +.. ----------------------------------------------------------------------------- + (c) Crown copyright Met Office. All rights reserved. + The file LICENCE, distributed with this code, contains details of the terms + under which the code may be used. + ----------------------------------------------------------------------------- + .. _support: Support @@ -5,9 +11,10 @@ Support Level 0- Self help & Peer Support --------------------------------- + Self-help and using the people around you should always be your first port of -call. Time spent here will likely solve your problem or at least better describe -it. +call. Time spent here will likely solve your problem or at least better +describe it. Consider contacting relevant Code, Configuration or Suite Owners. @@ -15,31 +22,35 @@ Level 1- GitHub Discussions --------------------------- If your problem persists, then describe and discuss it on GitHub Discussions: -* `LFRic `_ -* `UM `_ -* `JULES `_ -* `Mule `_ +* `LFRic `__ +* `UM `__ +* `JULES `__ +* `Mule `__ Level 2- Support Teams ---------------------- + If you still require assistance, then contact the appropriate support team. Please respect the clear demarcations between the scope for different teams. Core Capability Development Team (formally LFRic Team): - * Support windows for LFRic releases TBC + * Support windows for LFRic releases TBC -Simulation Systems and Deployment Team (formerly UM System Team and CRUM team) supports: - * UM, Shumlib and JULES versions released in the last 12 months, - * main version used in the Met Office Operational Suite. - * specific climate configurations .. todo - details of which configs +Simulation Systems and Deployment Team (formerly UM System Team and CRUM team) +supports: + + * UM, Shumlib and JULES versions released in the last 12 months, + * main version used in the Met Office Operational Suite. + * specific climate configurations ... Escalation ---------- + Sometimes things go wrong or different parties disagree. If discussions between -developer, code/config owners and reviewers cannot resolve the situation then it -should be escalated to the manager of the Simulation Systems and Deployment Team -in the first instance. They will consult appropriately to make the best balanced -and impartial decision. If this is unsuccessful, the ultimate (but very rarely -used) decision authority about code that may be committed to the trunk lies with -the Simulation Systems Governance Group. +developer, code/config owners and reviewers cannot resolve the situation then +it should be escalated to the manager of the Simulation Systems and Deployment +Team in the first instance. They will consult appropriately to make the best +balanced and impartial decision. If this is unsuccessful, the ultimate +(but very rarely used) decision authority about code that may be committed to +``main`` lies with the LFRic Apps Governance Group. diff --git a/source/FurtherDetails/who.rst b/source/FurtherDetails/who.rst index b0b2815b..98eed3fb 100644 --- a/source/FurtherDetails/who.rst +++ b/source/FurtherDetails/who.rst @@ -1,10 +1,18 @@ +.. ----------------------------------------------------------------------------- + (c) Crown copyright Met Office. All rights reserved. + The file LICENCE, distributed with this code, contains details of the terms + under which the code may be used. + ----------------------------------------------------------------------------- + Who's Who ========= + Every member of the community acts in one or more roles, depending on the work at hand. These roles often align to different teams for practical purposes. User ---- + Users, in general, are anticipated to: * Edit and manage rose suites via the roses repositories @@ -17,10 +25,11 @@ Notably, users in this context do not edit source code. Developer --------- + Developers, in general, build on the User role: * Write and edit source code -* Follow the Working Practices, even if their work is not intended for the trunk. +* Follow the Working Practices, even if their work is not intended for ``main``. * Manage their work to allow reasonable time for approvals and reviews. Notably, the Working Practices encourage and require engagement with other @@ -31,7 +40,9 @@ confidence in the models as a whole. Code Owner ---------- -Code Owners and their deputies take responsibility for defined parts of the codebase: + +Code Owners and their deputies take responsibility for defined parts of the +codebase: * Working knowledge of the code, and any supporting items such as metadata * Generally, focus will be on the scientific/functional aspects of the code, @@ -40,13 +51,16 @@ Code Owners and their deputies take responsibility for defined parts of the code * Oversee the general arc of development of their sections * Review and sign-off all changes to the code they own on an acceptable timescale -* In some cases they may make suggestions regarding changes outside of their section -* Often act as the Sci/Tech reviewer if their section is the primary focus of a change +* In some cases they may make suggestions regarding changes outside of their + section +* Often act as the Sci/Tech reviewer if their section is the primary focus of a + change .. _config_owner: Configuration Owner ------------------- + Configuration owners are people that have chosen to protect a model configuration (a specific arrangement on input settings and options) using the rose-stem test harness. @@ -55,85 +69,85 @@ rose-stem test harness. * Generally, focus will be on the scientific/functional aspect of the configuration, but also includes consideration of technical aspects with support -* Maintain, update and retire configurations to ensure they are relevant - and useful +* Maintain, update and retire configurations to ensure they are relevant and + useful * Will often be the owner or a experienced user of standalone science - evaluation suites that may be used to understand the magnitude of changes - in model evolution + evaluation suites that may be used to understand the magnitude of changes in + model evolution * Review and sign-off all changes to model output on an acceptable timescale -Importantly, rose-stem are the single source of truth for pass/fail protection of model evolution. +Importantly, rose-stem are the single source of truth for pass/fail protection +of model evolution. .. _scitech_reviewer: Sci/Tech Reviewer ----------------- -A Sci/Tech reviewer is assigned for every ticket and comprises the first stage -of review that considers the change as a whole. Further details are linked from -the :ref:`Working Practices page`. In some cases, the reviewer can -delegate parts of the work to another person. +A Sci/Tech reviewer is assigned for every pull request and comprises the first +stage of review that considers the change as a whole. Further details are found +in the :ref:`Scitech Reviewers Guide`. In some cases, the +reviewer can delegate parts of the work to another person. -Reviews should be turned around on a reasonable timescale and follow the SciTech -review guidance. +Reviews should be turned around on a reasonable timescale and follow the +SciTech review guidance. -It is the responsibility of the developer to identify and arrange the -Sci/Tech reviewer. Often they will be someone in the same team as the developer -or the most relevant code owner. +It is the responsibility of the developer to identify and arrange the Sci/Tech +reviewer. Often they will be someone in the same team as the developer or the +most relevant code owner. .. _code_reviewer: Code Reviewer ------------- -The Code Reviewer performs the 2nd stage of review for every ticket. -Further details are described :ref:`Working Practices page`. +The Code Reviewer performs the 2nd stage of review for every pull +request. Further details are described in the :ref:`Code Reviewers +Guide`. Reviews should be turned around on a reasonable timescale and follow the Code Review guidance. It is the responsibility of the developer to arrange the code reviewer. -**UM, LFRic Apps, JULES, UKCA and Shumlib** reviews are arranged by contacting -the Simulation Systems and Deployment Team. +Open Pull Requests will be assigned a Code Reviewer by the Simulation Systems +and Deployment team. -**LFRic Core** reviews are arranged by contacting the Capability Development Team, -or moving the ticket into "ready_for_code_review" state. Tickets in this state -are allocated reviewers at the weekly team meeting. +.. _simIT: + +Simulation IT +------------- -.. _ssd: +The Simulation IT department are responsible for the various codebases that comprise +the Simulation Systems. The department includes the following teams: -Simulation Systems and Deployment Team --------------------------------------- +Simulation Systems and Deployment Team: + The SSD Team oversees the curation of the repositories, working practices + and other supporting documentation, infrastructure and systems for all of + the Simulation Systems projects. -The Simulation Systems and Deployment Team are responsible for the various -codebases that comprise the Simulation Systems. + They provide support for the last 12 months of UM and LFRic Apps releases + to the Met Office and Partners. -* 4th line operational support to Met Office operational NWP system (excludes seasonal forecasting) -* Support for the last 12 months worth of UM, JULES, UKCA and shumlib releases to Met Office and partners -* Delivering releases and maintaining supporting infrastructure -* Overseeing the Working Practices and supporting documentation and mandating their use -* Curation of the trunks and supporting systems -* Coordination and collaboration with other system owners to provide overall seamless support -* Support for UM Climate Configurations -* Support and releases for the Met Office Coupling Infrastructure (MOCI) + The team can be contacted at ML-Simulation_Systems_and_Deployment@metoffice.gov.uk. -All efforts are delivered on a best-endeavors basis, with all requests being -triaged. Team members contribute to work outside of this project. +Core Capability Development Team: + The CCD team is responsible for the LFRic Core infrastructure code which + underpins several LFRic applications including the Momentum(r) atmosphere model. + The team is keen to ensure that application developments continue to follow + recommended standards such that applications benefit from the separation of + concerns principles and portable performance capabilities that the LFRic + core design and LFRic application development standards aim to support. -The team can be contacted at umsysteam@metoffice.gov.uk. + The team can be contacted at CoreCapabilityDevelopmentTeam@metoffice.gov.uk. -.. _cap_dev_team: +Tools and Collaborative Development Team: + The TCD Team is responsible for the development and integration of third + party tools with LFRic including Psyclone, XIOS and LFRic Inputs. -Core Capability Development Team --------------------------------- -The Core Capability Development Team are responsible for the LFRic Infrastructure to -support the Next Generation Modelling Systems. + The team can be contacted at ToolsCollabDevTeam@metoffice.gov.uk -The team can be contacted at corecapabilitydevelopmentteam@metoffice.gov.uk. -LFRic questions can also be directed to meto-lfric@metoffice.gov.uk. -.. todo: flesh out the description here .. _hpc_opt_team: @@ -143,15 +157,22 @@ HPC Optimisation Team The HPC optimistation team take a general lead in matters relating to compute performance of the UM, LFRic and other systems. -* Examine and improve the performance and scalability of the UM and coupled models. -* Develop and maintain GCOM, the communications layer used by the UM and other systems in the Met Office. +* Examine and improve the performance and scalability of the UM and coupled + models. +* Develop and maintain GCOM, the communications layer used by the UM and other + systems in the Met Office. * Development and support of the UM Packing/Unpacking?, Dump and I/O routines. * Benchmarking UM software for HPC evaluations/procurement. -* Act as 'code' owners for performance-related aspects of the UM, notably OpenMP and compiler directives +* Act as 'code' owners for performance-related aspects of the UM, notably + OpenMP and compiler directives The team can be contacted at Sci_Weath_hpc_opt@metoffice.gov.uk. Momentum Partnership Team ------------------------- -The Momentum Partnership Team is responsible for engagement and support with users and developers at Core and Associate `Momentum Partner `_ organisations. The team can be contacted at momentum_partnership@metoffice.gov.uk. +The Momentum Partnership Team is responsible for engagement and support with +users and developers at Core and Associate `Momentum Partner +`_ +organisations. The team can be contacted at +momentum_partnership@metoffice.gov.uk. diff --git a/source/Reviewers/codereview.rst b/source/Reviewers/codereview.rst index 30d4fcd4..6a8d89dd 100644 --- a/source/Reviewers/codereview.rst +++ b/source/Reviewers/codereview.rst @@ -1,101 +1,143 @@ +.. ----------------------------------------------------------------------------- + (c) Crown copyright Met Office. All rights reserved. + The file LICENCE, distributed with this code, contains details of the terms + under which the code may be used. + ----------------------------------------------------------------------------- + .. _code_review: -Code and System Review -====================== +Code Review +=========== + +.. tip:: + + GitHub documentation on the review process and interface: + `Reviewing Proposed Changes in a Pull Request `_ Purpose of the review --------------------- -The purpose of the code/system review is to analyse a change for its impact -and to ensure that all concerned parties are made aware of changes that are required. +The purpose of the Code Review is to analyse a change for its impact and +to ensure that all concerned parties are made aware of changes that are +required. Fundamentally this review is to ensure that the change is well thought-out and -that no system aspects have been missed. The review should be an active one and should question anything that is poorly coded. +that no system aspects have been missed. The review should be an active one +and should question anything that is poorly coded. + +Focus on the code, not the contributor; providing constructive, respectful and +actionable feedback. Critique the implementation, not the individual and always +explain the reasoning behind your suggestions. Reviewer responsibilities and checkpoints ----------------------------------------- -The Code/System review template exists to help you think through all the areas -of concern. A completed :ref:`review template