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.

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

Leap years in doy cyclic timeseries #439

Open
2 tasks done
atsiokanos opened this issue Jul 16, 2024 · 1 comment
Open
2 tasks done

Leap years in doy cyclic timeseries #439

atsiokanos opened this issue Jul 16, 2024 · 1 comment
Labels
documentation Improvements or additions to documentation v1.0 Activities related to v1.0

Comments

@atsiokanos
Copy link

Wflow version checks

  • I have checked that this issue has not already been reported.

  • I have checked that this bug exists on the latest version of Wflow.

Reproducible Example

Wflow.jl/src/io.jl

Lines 1167 to 1175 in 4becf7b

if length(times) == 12
months = Date(2000, 1, 1):Month(1):Date(2000, 12, 31)
return monthday.(months)
elseif length(times) == 365
days = Date(2001, 1, 1):Day(1):Date(2001, 12, 31)
return monthday.(days)
elseif length(times) == 366
days = Date(2000, 1, 1):Day(1):Date(2000, 12, 31)
return monthday.(days)

Current behaviour

Currently, wflow checks the length of the provided cyclic timeseries parameter. If the length is 366 then it assumes that it is always a leap year even in nonleap years. This is an issue in our forecast systems when we provide a ResFullFrac day-of-year value based on observed target levels in an operational setup, as the target volume timestamp we want to adjust is not mapped properly to the day of year fullfrac (if a 366 doy is provided in non-leap years or if we give a 365 doy in leap years). This can cause unnecessary reservoir spilling (and delays) in the short-term forecasts.

Desired behaviour

Consider combining both leap and non-leap years in cyclic doy timeseries. Perhaps an option is to map the doy from the forcing or so, and accept only 366 doy cyclic timeseries.

Additional Context

No response

@atsiokanos atsiokanos added the bug Something isn't working label Jul 16, 2024
@verseve verseve added documentation Improvements or additions to documentation v1.0 Activities related to v1.0 and removed bug Something isn't working labels Jul 17, 2024
@verseve
Copy link
Member

verseve commented Jul 17, 2024

After having a chat with @JoostBuitink and @atsiokanos we concluded that this is not a bug, but that the documentation can be improved on how this is internally handled. The 365-day and 366-day cyclic time series can both be used for leap and non-leap years. When a user provides a 365-day cyclic time series, the value at day (index) 59 is used for Feb. 28 and 29. The mapping between cyclic time and current model time in wflow is based on month-day tuples (e.g. (2, 28)), and this is why the assumption that day of year and index are equal is not correct. Finally, good to include in the documentation that wflow expects right forcing time stamps (at the end of a time step) while for cyclic time series time stamps this is left (at the start of a time step).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation v1.0 Activities related to v1.0
Projects
None yet
Development

No branches or pull requests

2 participants