Tests for date formats, with conversion for POSIXlt

[josephguillaume]

It turns out chron conversion is only necessary for POSIXlt according to these tests, and conversion is therefore added to the hydromad constructor.

All Submissions:

• [x ] Have you followed the guidelines in our CONTRIBUTING document?
• [ x] Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Closes #31

Types of changes

• New feature (non-breaking change which adds functionality)

1. Have you ensured there are no conflicts with major packages (e.g. tidyverse, MASS) or base R packages?
2. Does your new feature require documentation, and if so, have you updated the documentation? - simply avoids an error
3. Have you added tests for your features, and do your features pass all tests?
4. Have you linted your code with lintr locally prior to submission?
5. Have you styled your code with styler locally prior to submission?

Changes to Core Features:

• [x ] Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you added tests for your core features and do your core features pass all tests?
• Have you successfully run tests with your changes locally?
• Does your code run on all major platforms (Windows, macOS, Linux)?
• Have you demonstrated backwards compatibility and compatibility with the current development version, or discussed a potential break in backwards compatibility?

[josephguillaume]

Added checks for warmup period and length of data (affecting suitability of objective) to hydromad. This partially addresses #171 and #170, which will also require an example in the revised tutorial #38

[josephguillaume]

/document

[josephguillaume]

@WillemVervoort and @buzacott - does this look ok to you?

I was unable to create a reproducible example that causes errors with POSIXct, so do let me know if you have one.

Note that the checks are only run when a hydromad object is created, not when it is updated, but I think this would already be helpful for beginners?

[buzacott]

I can't fit anything with a POSIXct index. Does it also need to be coerced to chron?

library(hydromad)

data("Corin")

# Date
Corin = Corin[1:365,]
mod <- hydromad(DATA = Corin,
                sma = 'gr4j',
                routing = 'gr4jrouting')
fit <- fitByOptim(mod)
# Works

# POSIXlt
index(Corin) = as.POSIXlt(strftime(index(Corin)), tz='Australia/Queensland')
mod <- hydromad(DATA = Corin,
                sma = 'gr4j',
                routing = 'gr4jrouting')
fit <- fitByOptim(mod)
# Works

# POSIXct
index(Corin) = as.POSIXct(strftime(index(Corin)), tz='Australia/Queensland')
mod <- hydromad(DATA = Corin,
                sma = 'gr4j',
                routing = 'gr4jrouting')
fit <- fitByOptim(mod)
# Doesn't work, just hangs

# Try with hourly data
data(Wye)
class(index(Wye))
# [1] "POSIXct" "POSIXt" 
Wye$E = 1 # add ET
mod <- hydromad(DATA = Wye,
                sma = 'gr4j',
                routing = 'gr4jrouting')
fit <- fitByOptim(mod)
# Doesn't work

[josephguillaume]

Thanks for the examples!
It turns out the root cause in these cases is that frequency is not set correctly. By replacing the index, the existing attribute of frequency=1 is not modified, and because a unit of 1 is a second using POSIXct, as.ts fills in NAs between the timesteps, e.g.

index(Corin) = as.POSIXct(strftime(index(Corin)), tz='Australia/Queensland')
as.ts(head(Corin,n=2))

as.ts is being called at every model run in gr4j and the NA-filling takes time and a lot of memory (86400x the original number of rows!), which is why it seems like it hangs.

If instead, a zoo object is constructed without a frequency, it works:

Corin=zoo(coredata(Corin),order.by=as.POSIXct(strftime(index(Corin)), tz='Australia/Queensland'))

If I want to keep a zooreg object, I can also set the frequency correctly:

index(Corin) = as.POSIXct(strftime(index(Corin)), tz='Australia/Queensland')
frequency(Corin)<-1/86400
as.ts(head(Corin,n=2))

If I convert to chron, a unit of 1 is a day, so the frequency of 1 is correct

index(Corin)<-chron::as.chron(index(Corin))
as.numeric(head(index(Corin)))

If I somehow got the frequency wrong with chron, I would again get the NA-filling behaviour:

frequency(Corin)<-2
as.ts(head(Corin,n=2))

So what we have is a usability issue rather than a pure incompatibility with POSIXct.
Another solution is to encourage users not to use zooreg unless they know what they're doing. as.ts handles zoo objects correctly if they don't already have a frequency, e.g. :

Corin <- as.zoo(Corin)
Corin = Corin[1:365,]
index(Corin) = as.POSIXct(strftime(index(Corin)), tz='Australia/Queensland')
as.ts(head(Corin,n=2))

Edit: the datasets in hydromad that are zooreg with incorrect frequencies obviously need to be changed regardless!
Edit 2: as.ts works on zoo because it estimates the frequency as the GCD of the diff of time steps (zoo:::frequency.zoo). This obviously doesn't always work.

[josephguillaume]

Maybe a simple solution is to give a warning if (end-start)*frequency is more than e.g. 10% greater/smaller than the number of rows in the dataset.

[josephguillaume]

Also revisit error in nseStat, as described in merged PR: #187 (comment)