Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create FatesConstantMod to hold global constants #136

Merged
merged 2 commits into from
Oct 13, 2016
Merged

Create FatesConstantMod to hold global constants #136

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0498d5e
Create FatesConstantMod to hold global constants
bandre-ucar Oct 11, 2016
a6aaaa4
Remove special value constants from FatesConstantsmod
bandre-ucar Oct 12, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions components/clm/src/ED/main/FatesConstantsMod.F90
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
module FatesConstantsMod
! This module is used to define global _immutable_ data. Everything in
! this module must have the parameter attribute.

implicit none

public

! kinds
integer, parameter :: fates_r8 = selected_real_kind(12) ! 8 byte real
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we strip this down to something shorter like "f_r8" or "fr8"? I feel like this will be used so often that we can reduce total letter count with an abbreviation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you use it, you can still alias it to r8 with something like 'use FatesConstantsMod, only : r8 => fates_r8'. See how it is used in the history module. The actual declaration should have a meaningful, unambiguous name.

There are no bonus points in software engineering for 'reducing the total letter count'. The compiler doesn't care. Code is written once, refactored a few times, and read many, many, many times by human beings. Meaningful names without cryptic abbreviations optimize for the most common use case. In science, it is often humans with little to no training and interest in software as an end. The more clear the code is, the less time it takes to ramp up, and the fewer mistakes they will make. Plus, all modern editors have some form of autocompletion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the solution of
use FatesConstantsMod, only : r8 => fates_r8
since it only requires changing one line per module...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not seeing the alias was an over-site on my part. Try to relax a little bit @bandre-ucar. I have arguments that I can go into that validate why I made the suggestion I did, but I'm not. The way your are proposing to go forward is fine. The important thing is that we create a culture on these forums where people feel welcomed to interact and know that their input will be valued.


! string lengths
integer, parameter :: fates_avg_flag_length = 3
integer, parameter :: fates_short_string_length = 32
integer, parameter :: fates_long_string_length = 199

! various magic numbers
real(fates_r8), parameter :: fates_special_value = 1.0e36_fates_r8 ! special value for real data, compatible with clm.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have something similar in clm_fates%init(). A call to set_fates_ctrlparms('hio_ignore_val',rval=spval) transfers the host model's definition of the special value to FATES. It transfers the special value to be used exclusively as a flag to initializing unset history output arrays.

I was thinking it would be nice to have a special section where we house constants that are dictated by the host, I am agnostic as to where that could be located.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added spval on a whim because I that was what started this PR, even though I'm not using it. It's not immediately obvious if we need to distinguish between fates_spval and host_spval (seems useful, but maybe not). I'm removing fates specific special values until needed.

integer, parameter :: fates_int_special_value = -9999 ! keep this negative to avoid conflicts with possible valid values

end module FatesConstantsMod
19 changes: 10 additions & 9 deletions components/clm/src/ED/main/HistoryIOMod.F90
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
Module HistoryIOMod


use shr_kind_mod , only : r8 => shr_kind_r8
use FatesConstantsMod, only : r8 => fates_r8
use FatesConstantsMod, only : fates_avg_flag_length, fates_short_string_length, fates_long_string_length
use FatesGlobals , only : fates_log
use EDTypesMod , only : cp_hio_ignore_val
use pftconMod , only : pftcon
Expand Down Expand Up @@ -127,7 +128,7 @@ Module HistoryIOMod
! This structure is not allocated by thread, but the upper and lower boundaries
! of the dimension for each thread is saved in the clump_ entry
type iovar_dim_type
character(len=32) :: name ! This should match the name of the dimension
character(fates_short_string_length) :: name ! This should match the name of the dimension
integer :: lb ! lower bound
integer :: ub ! upper bound
integer,allocatable :: clump_lb(:) ! lower bound of thread's portion of HIO array
Expand All @@ -150,7 +151,7 @@ Module HistoryIOMod

! This structure is not multi-threaded
type iovar_dimkind_type
character(len=32) :: name ! String labelling this IO type
character(fates_short_string_length) :: name ! String labelling this IO type
integer :: ndims ! number of dimensions in this IO type
integer, allocatable :: dimsize(:) ! The size of each dimension
logical :: active
Expand All @@ -162,15 +163,15 @@ Module HistoryIOMod

! This type is instanteated in the HLM-FATES interface (clmfates_interfaceMod.F90)
type iovar_def_type
character(len=32) :: vname
character(len=24) :: units
character(len=128) :: long
character(len=24) :: use_default ! States whether a variable should be turned
character(len=fates_short_string_length) :: vname
character(len=fates_short_string_length) :: units
character(len=fates_long_string_length) :: long
character(len=fates_short_string_length) :: use_default ! States whether a variable should be turned
! on the output files by default (active/inactive)
! It is a good idea to set inactive for very large
! or infrequently used output datasets
character(len=24) :: vtype
character(len=1) :: avgflag
character(len=fates_short_string_length) :: vtype
character(len=fates_avg_flag_length) :: avgflag
integer :: upfreq ! Update frequency (this is for checks and flushing)
! 1 = dynamics "dyn" (daily)
! 2 = production "prod" (prob model tstep)
Expand Down