Code review for changeset 27 / hasod

[pesei]

Namelist input

changeset:27 by hasod contains:

• Implemented optional namelist input for COMMAND, RELEASES, SPECIES, AGECLASSES,OUTGRID,OUTGRID_NEST,RECEPTORS

• Implemented com_mod switch nmlout to write input files as namelist to the output directory (.true. by default)

• Proposed updated startup and runtime output (may change back to previous info if desired)

This ticket is also a place to discuss the proposed new standard output of a run.

[pesei]

I'll start to give some comments here.

1. variable names. Let's follow the FpCodingStandard and instead of verbosity use iverbosity etc.

2. Let's call the namelist files *.nl instead of *.namelist - usually extensions are short, 2-4 chars. And then maybe for nmlout nlout or (to use l for logicals) lnlout.

3. Let's minimise redundancy in the code by replacing code blocks such as

       if (verbosity.gt.0) then 
         write(*,*) '# readoutgrid_nest' 
       endif 
   

   by a one-line subroutine call such as

       call verboutput (iverbosity, '# readoutgrid_nest')
   

   and similarly wherever output structures repeat themselves. Or, to be even more flexible, with iverbositymin to be used as the minimum verbosity level required to cause the output.

       call verboutput (iverbosity,iverbositymin, '# readoutgrid_nest')
   

[hasod]

OK with the proposed changes.

About #3: I think that's a great start to collect all (error)messages in one module, there is a lot of redundancy in the code and legacy error messages such as "not found on disk". Maybe this is already in the whishlist?

[pesei]

More suggestions:

Include nml in variable names for namelists.

See also #115 for a namelist-related bug.

[pesei]

Notes from the code review:

Generic remarks about the namelists

• rename *.namelist files to *.nl (standard extension - 1 to 4 chars)
• rename nmlout to lnlout (let's start logical variables with l)

FLEXPART.f90

• shall we replace getarg and iargc by Fortran2003 names?
• command line interface issues:
  • pathfile <= 256 chars not checked
  • mixture of positional parameter and named switch, too complicated
  • only verbosity OR infostring parameter can be given - why?
  • infoflag: nothing implemented yet
  • Should have a simple and expandable logic -> NEED FOR DISCUSSION
• simple verbosity messages: moved to subroutine verboutput by pesei
• verbosity message wrt to system time: > MOVE TO A SUBROUTINE
• simplify syntax of the writeheader calls, remove double calls NOTE that netCDF is not inplmented for surf_only
• info_flag .eq. 1 stops code before calling timemanager -> so what is its purope?
• moving CONGRATULATIONS message after the last system clock message because we need a unique string at the very end for eady verification of successful termination of the programme

timemanager.f90

• decay of deposits: loops in inefficient order, exponential function evaluated in innermost loop > CLEAN THIS UP
• introduce call verboutput as above; system time info not modified yet
• there are places with if (iverbosity .gt. 0) and if (iverbosity .eq. 1). I am treating the second case like the first one - otherwise strange logic
• timemanager> called concoutput_surf removed - everywhere else we have verbosity output only before call
• name the loop do j=1,numpart - if we replace loop end statement numbers with ENDDO, we should name long loops!
• if (xmassfract.lt.0.0001) then ! terminate all particles carrying less mass make this constant a parameter in parmod > TODO
• itra1(j)=-999999999 make this constant a parameter in parmod > TODO
• modified some verbosity messages
• regular output of gridtotalunc,wetgridtotalunc,drygridtotalunc has been removed, only itime,numpart remaining. IS THIS WHAT WE WANT?
• there are statements for deallocation at the end of timemanager. However, according to ​http://www.fortran90.org/src/best-practices.html, When using allocatable arrays (as opposed to pointers), Fortran manages the memory automatically and it is not possible to create memory leaks. and When the ... symbol goes out of scope, Fortran will deallocate it.

  So I am commenting out these deallocate commands.
  

concoutput.f90

• replace nclassunc by Fortran90 syntax
• remove lonely statement no 125
• clean up messy indentation, name long DO loops
• > ALL THESE OUTPUT SUBROUTINES ARE CANDIDATES FOR REMOVING CODE REDUNDANCY!!! note that for the flexRISK dose code, I have already useful modules for that

[pesei]

Notes from the code review [cont.1]:
(not all remarks here were introduced in the latest changeset of hasod, some are older, but I still list them here)

• There are many places with path()(1:length()) - should probably all be replaced by trim( path() )

outgrid_init.f90

• remove all .eqv..true. - unnecessary!
• fix some unnecessary continuation lines & indents
• there are loops in inefficient order > CHECK

read_ageclasses.f90

• has hardcoded constants
• some variable names changed as in readreleases.f90

calcpv.f90, calcpv_nests.f90

• not reviewing these subroutines as they have to replaced by more efficient code, see ticket:77
• however, I noticed that new 3D fields were introduced - that's also a memory issue. In the future, make sure that PV-related fields and calculations are only activated if they are explicity demanded!

readwind.f90, readwind_nests.f90

• add missing parameter iret in call to grib subr
• add some verbosity messages
• add GRIB2 coding for deta/dt dp/deta used by ZAMG
• proper indenting
• fix inefficient loop order
• remove unused labe 5

     ! CALCULATE USTAR AND SSHF USING THE PROFILE METHOD
     ! As ECMWF has increased the model resolution, such that now the first model
     ! level is at about 10 m (where 10-m wind is given), use the 2nd ECMWF level
     ! (3rd model level in FLEXPART) for the profile method
  

  the level should not be hardcoded - maybe someone will use older data!

readcommand.f90

• remove output saying "update to namelist format - I don't think regular text format won't be supported in the future
• change output nml file name to COMMAND.nml

readpaths.f90

• simplify syntax for reading nest paths
• add check for lastchar to nest paths
• use trim() and len_trim()

com_mod.f90

• (and elsewhere) inconsistent spelling of new variable parID, sometimes written as parId. Fortran is not case specific and in most places therefore we just write lowercase. And it is Integer, thus I suggest to make it idpar or if you want IDpar.

gridcheck.f90

• rename all integer variables to i...
• some nicer formatting of some code, indenting
• getting rid of outdated CHANGE CDROM message & interactive query
• what about all the other gridcheck subroutines? candidates for code redundancy elimination! Communicate with CTBTO contractors as they might remove some of them.

[pesei]

Notes from the code review [cont.2]:

---

Global changes:
remove all .eqv..true. (not needed in Fortran)

---

concoutput.f90 (additionally)

• introduce more f90 syntax

concoutput_nest.f90

• convert loops to f90 array assignments (there were even unnecessary multiple loops for creceptor(i,ks)=0. over kp=1,maxpointspec_act, and loops in wrong order!)
• add reinitialisation of wetgridunc and drygridunc if new variable ldep_incr true
• remove reinitalisation of creceptor, this is alread done in concoutput.f90

concoutput_surf.f90

• convert loops to f90 array assignments (there were even unnecessary multiple loops for creceptor(i,ks)=0. over kp=1,maxpointspec_act, and loops in wrong order!)
• add reinitialisation of wetgridunc and drygridunc if new variable ldep_incr true

netcdf_output_mod.f90

• adapt code layout to usual Flexpart standards (no wide lines etc.)
• note that there is a lot of redundancy with standard output. In the future, try to eliminate that.
• is a module with several subroutines, > 1300 lines of code. Do we really want all in one module? If so, use include or (Fortran 2008) submodules in separate files?* netcdf_output_mod.f90
• rename nf90_err to nc_err so it does not look like standard netcdf lib

timemanager.f90

• code layout
• naming long loops
• reordering inefficient loops
• introduce call verboutput

[pesei]

Notes from the code review [cont.3]:

---

Global changes:
use trim(path(...)) instead of path(1:length(...))(...) everywhere

---

readageclasses.f90

• some variable names changed as in readreleases.f90 !
• catch nageclass>maxage properly

readpartpositions.f90

• code layout
• NOTE check logic of loops 100/200, maybe simplify
• NOTE has hardcoded tiny of 1e-5
• NOTE numpointin.ne.numpoint - changed from ERR to WARN

ew.f90

• rewritten using e-x instead of /10.x
• comment in English, replace x by t (temperature)

advance.f90

• NOTE that Petterssen part has redundancies!!
• more problems, will probably go into one or more separate tickets
• fix mixture of real and dp in call to funtion mod (already in r36)

readlanduse.f90

• reorder loops
• rename variables
• integer,parameter instead of integer constants
• rename surfdata.t to z0_data.txt and make it readable without explicit fmt, but keep compatibility to old input

assignland.f90

• bring nested loops into efficient order
• rename some variables
• make xlon0lu,ylat0lu real

conv_mod.f90 dynamic_viscosity.f90 interpol_mod.f90 readwind.f90 psim.f90

• trivial code layout changes, such as spaces

readdepo.f90

• integer parameter numseas instead of integer constant=5
• rename surfdepo.t to sfc_depo_params.txt, but keep compatibility to old input
• NOTE: has hardcoded large value of 1e25 and other constants
• replace loops by f90 :,:
• NOTE some constants need to be checked, look suspicious

boundcond_domainfill.f90

• replace 48./29.*ozonescale/10.**9 by new parameter ozonefactor defined in par_mod.f90
• replace all /2. by 0.5*
• rename long loops
• some cosmetic changes

[pesei]

Notes from the code review [cont. 4]:

boundcond_domainfill.f90

• replace 48./29.*ozonescale/10.**9 by new parameter ozonefactor defined in par_mod.f90
• replace all /2. by 0.5*
• rename long loops
• some cosmetic changes

calcfluxes.f90

• replace all /2. by 0.5*
• replace int( x+0.5 ) by nint(x); note that negative arguments don't occur, so it is equivalent and shorter
• BUG real(nxmin1)-1.e5 in line 149. Changed this to real(nxmin1)-1.e-5
• NOTE : issue WARN if grid index is outside expected range? (presently: skip loop silently)

calcmatrix.f90, calcmatrix_nfs.f90

• initalisation of fmassfrac, replace inefficient loop by f90 :,:
• replace /ga by *ga_inverse
• rename summe to sum

calcpar.f90, calcpar_nfs.f90

• name long loops
• NOTE code to calculate geomtric height of model levels comes in several subroutines, candidate for redundancy removal
• NOTE calcpv to be called only if PV is required

centerofmass.f90, clustering.f90

• minor code layout change

conccalc.f90

• code layout, naming loops
• NOTE kernel width calculation has impicit assumption for latitude, will be improper in very high latitude

convmix.f90

• code layout

convmix_nfs.f90

• code layout
• WARN does not use eps in IF statements as convmix.f90 does
• WARN also, it lacks USE flux_mod

coordtrafo.f90

• NOTE has eps as 1.e-6, it will be important to have a consistent eps system, and only where really needed
• code layout and simplification

distance.f90

• give rearth in km instead of m, and remove division

by 1000. in the line distance=...

• replace /prd with *rpd (with rpd being defined inverse),

its faster

• WARN for angular distance <1.8' (ca. 3 km) distance=0!
• subr uses kind=dp, but the difference rlon1-rlon2 where

it probably would be most important was sngl !! corrected

• no source of code is given, but it seems to imported from

NOAA, added a note with link

distance2.f90

• give rearth in km instead of m, and remove division

by 1000. in the line distance=...

• remove unused parameter pi
• WARN for angular distance <3.e-4 (ca. 2 km) distance=0!

NOTE: likely source of code is subroutine gcdist found e.g. on ​http://www.nco.ncep.noaa.gov/pmb/codes/nwprod/lib/ip/v2.0.0/sorc/src/
NOTE: distance is used in plumetraj.f90 only, distance2 in clustering.f90 only. Maybe unify.