CH-Earth / summa

Structure for Unifying Multiple Modeling Alternatives:
http://www.ral.ucar.edu/projects/summa
GNU General Public License v3.0
80 stars 106 forks source link

Outputted Time Variable is Inconsistent, affecting input to mizuroute #528

Open KyleKlenk opened 1 year ago

KyleKlenk commented 1 year ago

Bug Reports

Here is an example of the time variable output I get from running SUMMA: 567993600.00000000, 567997199.99998660,568000800.00001340, 568004400.00000000, 568007999.99998660, 568011600.00001340 ....

I have looked in the code and have traced this behaviour to: read_force.f90:194

do iGRU=1,size(gru_struc)
  do iHRU=1,gru_struc(iGRU)%hruCount
   forcStruct%gru(iGRU)%hru(iHRU)%var(iLookFORCE%time) = (currentJulday-refJulday)*secprday
  end do  ! looping through HRUs
 end do  ! looping through GRUs

Here currentJulDay is represented as a decimal and causes the resulting time value to contain a decimal as well.

Is this expected behaviour? It looks like seconds since is the only format the time variable is output with as per:

 forc_meta(iLookFORCE%time)                  = var_info('time'    , 'time since time reference'                         , 'seconds since 1990-1-1 0:0:0.0 -0:00', get_ixVarType('scalarv'), iMissVec, iMissVec, .false.)

Is it okay to add a fix that rounds the number of seconds to the nearest whole number? It looks like this can be done in read_force.f90:194 or when writing the output at modelwrite.f90:210.

Or is there a way to configure SUMMA to avoid the time variable output behaviour I am experiencing?

martynpclark commented 1 year ago

Hi Kyle, as we discussed over slack, the rounding fix should test the assumption that the time units are seconds. If not, and someone changes the time units (e.g., to days), then the rounding will no longer be valid and the error may be difficult to find.

KyleKlenk commented 1 year ago

Would it make sense to add an additional variable to globalData.f90 and case statements set for this value in the output? or would an additional variable to the forcStruct make more sense?

I also think maybe a warning message at the start of the code may be helpful for this? Just testing against a variable might still be tricky to find if it is unknown this setting/variable exists. I am just thinking for my suggestion about putting in globalData.f90, as I would easily overlook this setting myself.

I don't see any existing variables that we could use to test against currently. If there is one I will use it.