WIP: feat: switch from time-rs to chrono and implement non-daily timesteps #115

Batch21 · 2024-02-15T14:44:14Z

No description provided.

Batch21 · 2024-02-16T12:20:31Z

@jetuk would be good to get some feedback on this. One potential change I've just noticed is that the virtual storage state will need updating to store timestep length, otherwise there will be no way to know the amount of flow to recover for each timestep.

jetuk · 2024-02-16T21:12:46Z

One potential change I've just noticed is that the virtual storage state will need updating to store timestep length, otherwise there will be no way to know the amount of flow to recover for each timestep.

I'm not sure I understand? The current implementation uses Timestep.days() the same as the regular storage state?

jetuk

I've had a quick read through and left some comments.

pywr-core/src/models/mod.rs

pywr-core/src/parameters/discount_factor.rs

pywr-core/src/parameters/profiles/monthly.rs

jetuk · 2024-02-16T21:38:30Z

pywr-core/src/parameters/profiles/monthly.rs

@@ -76,13 +86,16 @@ impl Parameter for MonthlyProfileParameter {
        let v = match &self.interp_day {
            Some(interp_day) => match interp_day {
                MonthlyInterpDay::First => {
+                    let next_month = (timestep.date.month() % 12) + 1;


Is this correct? .month() is 1-based here?

// Returns the zero-based next month let next_month0 = (timestep.date.month0() + 1) % 12; let last_value = self.values[next_month0 as usize];

chrono::Month?

jetuk · 2024-02-16T21:39:52Z

pywr-core/src/parameters/profiles/monthly.rs


                    interpolate_first(&timestep.date, first_value, last_value)
                }
                MonthlyInterpDay::Last => {
-                    let first_value = self.values[timestep.date.month().previous() as usize - 1];


The time API is much nicer here.

pywr-core/src/recorders/aggregator.rs

jetuk · 2024-02-16T21:47:26Z

pywr-core/src/timestep.rs

-    start: Date,
-    end: Date,
-    timestep: Duration,
+    start: NaiveDateTime,


This was a concious decision to add time into the time-stepping?

Yes, to make sub-daily timesteps possible. If we didn't want to allow these at this point we could revert to a NaiveDate

pywr-core/src/recorders/aggregator.rs

pywr-core/src/virtual_storage.rs

jetuk · 2024-02-16T21:58:45Z

pywr-schema/src/model.rs

@@ -48,20 +49,40 @@ impl From<pywr_v1_schema::model::Timestep> for Timestep {
    }
 }

+#[derive(serde::Deserialize, serde::Serialize, Clone, Copy)]
+pub struct DateTimeComponents {


This is because you can't deserialise a string without a time component if the type is a NaiveDateTime?

yes, at a minimum I think the string has to have hour and minute components

We could leave it as a date that automatically gets added 00:00:00. Or make a untagged enum for a Date string or this date-time struct?

I've gone with an untagged enum which seems to work.

Batch21 · 2024-02-17T10:52:31Z

One potential change I've just noticed is that the virtual storage state will need updating to store timestep length, otherwise there will be no way to know the amount of flow to recover for each timestep.

I'm not sure I understand? The current implementation uses Timestep.days() the same as the regular storage state?

In recover_virtual_storage_last_historical_flow the current timestep is used but its length might be different to the timestep when the flow added to the storage state?

jetuk · 2024-02-17T11:18:50Z

I understand. It seems like the history would need a method to pop off mean flow over a period equal to the current time-step duration. And yes, therefore it would need to keep a record of the durations associated with each history entry. I think generally this will be ok as it is because time-steps tend to be similar in duration if bit identical. Therefore, note it as an issue and we'll fix it in another PR.

…

On Sat, 17 Feb 2024, 10:52 James Batchelor, ***@***.***> wrote: One potential change I've just noticed is that the virtual storage state will need updating to store timestep length, otherwise there will be no way to know the amount of flow to recover for each timestep. I'm not sure I understand? The current implementation uses Timestep.days() the same as the regular storage state? In recover_virtual_storage_last_historical_flow the current timestep is used but its length might be different to the timestep when the flow added to the storage state? — Reply to this email directly, view it on GitHub <#115 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABDEF3TMUAI3V26WW4S72ULYUCDXXAVCNFSM6AAAAABDKMMTA6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNBZHEZTINJTGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

jetuk · 2024-02-18T11:13:20Z

I wonder if it would be beneficial to have two different TimeDomain types? One for fixed time-step another for variable? I'm not sure how this would interact with things like the virtual storage history though.

…es and naivedatetimes

This wraps the Chrono TimeDelta type and provides a fractional_days method that can be used for metric aggregation.

Batch21 · 2024-02-21T16:08:16Z

@jetuk I think this is ready for you to take another look

jetuk

Some quick comments on the latest changes. I'll do a separate review of the whole thing.

jetuk · 2024-02-21T21:07:17Z

pywr-core/src/timestep.rs

+    }
+}
+
+impl PywrDuration {


It would be good to add some simple doc strings to these methods and the struct itself.

jetuk · 2024-02-21T21:08:10Z

pywr-core/src/timestep.rs

+    }
+
+    pub fn whole_days(&self) -> Option<i64> {
+        if self.fractional_days() % 1.0 == 0.0 {


Does this pass clippy checks of comparing against floating point values?

I don't get a warning locally. Is there going to be an issue with floating point precision here? Might be better to use the float_cmp crate?

I was possible expecting clippy to show this: https://rust-lang.github.io/rust-clippy/master/index.html#/float_cmp_const

However, it is part of the "restriction" group which means it should be considered case-by-case. The question is whether there's any reasonable output of fractional_days() which we expect to be treated as a whole day, but would not be by this test? Leap seconds? Are those still a thing?

If there's not already just add a test for this method.

@Batch21 I did a bit of reading here. I think modulo on floats is a bit weird sometimes. It might be better to do the modulo on the num_seconds() as a i64.

Something like this might be better?

const SECS_IN_DAY: i64 = 60 * 60 * 24; pub fn whole_days(&self) -> Option<i64> { if self.0.num_seconds() % SECS_IN_DAY == 0 { Some(self.0.num_days()) } else { None } } pub fn fractional_days(&self) -> f64 { self.0.num_seconds() as f64 / SECS_IN_DAY as f64 }

jetuk · 2024-02-21T21:15:30Z

pywr-core/src/recorders/aggregator.rs

@@ -47,7 +48,7 @@ impl AggregationFrequency {
        let mut sub_values = Vec::new();

        let mut current_date = value.start;
-        let end_date = value.start + value.duration;
+        let end_date = value.start + *value.duration.time_delta();


One alternative here would be to implement Add for PywrDuration with rhs = NaiveDateTime

jetuk · 2024-02-22T00:01:31Z

pywr-core/src/models/mod.rs

-    pub fn from(timestepper: Timestepper, scenario_collection: ScenarioGroupCollection) -> Self {
-        Self {
-            time: timestepper.into(),
+    pub fn from(timestepper: Timestepper, scenario_collection: ScenarioGroupCollection) -> Result<Self, PywrError> {


Rename this to try_from

jetuk · 2024-02-22T00:03:31Z

pywr-core/src/parameters/profiles/monthly.rs

@@ -5,8 +5,8 @@ use crate::state::{ParameterState, State};
 use crate::timestep::Timestep;
 use crate::PywrError;
 use std::any::Any;
-use time::util::days_in_year_month;
-use time::Date;
+


Remove empty line

jetuk · 2024-02-22T00:32:09Z

pywr-core/src/parameters/py.rs

@@ -120,7 +121,7 @@ impl Parameter for PyParameter {
                py,
                timestep.date.year(),
                timestep.date.month() as u8,
-                timestep.date.day(),
+                timestep.date.day() as u8,


I think we can use the chrono feature in pyo3 to pass the date directly as an argument.

jetuk · 2024-02-22T00:32:18Z

pywr-core/src/parameters/py.rs

@@ -154,7 +155,7 @@ impl Parameter for PyParameter {
                    py,
                    timestep.date.year(),
                    timestep.date.month() as u8,
-                    timestep.date.day(),
+                    timestep.date.day() as u8,


jetuk · 2024-02-22T00:38:14Z

pywr-core/src/recorders/hdf.rs

-            month: ts.date.month().into(),
-            day: ts.date.day(),
+            month: ts.date.month() as u8,
+            day: ts.date.day() as u8,


Do we need to support saving the HMS data in the HDF file?

jetuk · 2024-02-22T00:39:38Z

pywr-core/src/test_utils.rs

 }

 pub fn default_time_domain() -> TimeDomain {
-    default_timestepper().into()
+    TimeDomain::try_from(default_timestepper()).unwrap()


default_timestepper().try_into().unwrap() should also work here and not require the TimeDomain type to written twice.

jetuk · 2024-02-22T00:40:27Z

pywr-core/src/timestep.rs

 use pyo3::prelude::*;
 use std::ops::Add;
-use time::{Date, Duration};
+


Remove empty line.

jetuk · 2024-02-22T00:41:49Z

pywr-core/src/timestep.rs

            timesteps.push(current);
            current = next;
        }
        timesteps
    }
+
+    fn generate_timesteps_from_frequency(&self, frequency: String) -> Result<Vec<Timestep>, PywrError> {


Can we add a docstring here. Also it would be more idiomatic to use frequency: &str.

jetuk · 2024-02-22T00:48:46Z

pywr-core/src/virtual_storage.rs

@@ -352,7 +375,8 @@ mod tests {
        let recorder = AssertionFnRecorder::new("link-1-flow", Metric::NodeOutFlow(idx), expected, None, None);
        network.add_recorder(Box::new(recorder)).unwrap();

-        let model = Model::new(default_timestepper().into(), network);
+        let domain = ModelDomain::try_from(default_timestepper()).unwrap();


let domain = default_timestepper().try_into().unwrap(); is slightly shorter.

jetuk · 2024-02-22T00:53:16Z

pywr-python/src/lib.rs

-        let start =
-            Date::from_calendar_date(start.get_year(), start.get_month().try_into().unwrap(), start.get_day()).unwrap();
-        let end = Date::from_calendar_date(end.get_year(), end.get_month().try_into().unwrap(), end.get_day()).unwrap();
+        let start = DateType::Date(


Should we support datetime in this API as well?

Batch21 · 2024-02-23T22:51:44Z

@jetuk I think the latest commits address all your comments

jetuk

LGTM. Good update!

feat: switch from time-rs to chrono and implement non-daily timesteps

3e2fe43

Batch21 changed the title ~~feat: switch from time-rs to chrono and implement non-daily timesteps~~ WIP: feat: switch from time-rs to chrono and implement non-daily timesteps Feb 15, 2024

Batch21 added 2 commits February 16, 2024 11:51

fix python test + remove some unwraps

d751aeb

Merge branch 'main' into chrono

d035095

Batch21 requested a review from jetuk February 16, 2024 12:15

jetuk reviewed Feb 16, 2024

View reviewed changes

Batch21 added 2 commits February 18, 2024 20:21

impl TryFrom<Timestepper> for ModelDomain and TimeDomain + remove unwrap

5174881

use untagged enum for model start/end attributes that allows naivedat…

4dd53eb

…es and naivedatetimes

jetuk mentioned this pull request Feb 20, 2024

Build Python ABI3 wheels #96

Closed

Batch21 added 2 commits February 21, 2024 09:18

Merge branch 'main' into chrono

3138ca3

feat: Add a PywrDuration struct

427f432

This wraps the Chrono TimeDelta type and provides a fractional_days method that can be used for metric aggregation.

jetuk requested changes Feb 21, 2024

View reviewed changes

jetuk requested changes Feb 22, 2024

View reviewed changes

Batch21 added 2 commits February 23, 2024 22:24

more review response

ef3fd54

Merge branch 'main' into chrono

7ac9e6f

jetuk approved these changes Feb 24, 2024

View reviewed changes

jetuk merged commit 8db2928 into main Feb 24, 2024
18 checks passed

jetuk mentioned this pull request Feb 24, 2024

Sub-daily profile interpolation #123

Closed

WIP: feat: switch from time-rs to chrono and implement non-daily timesteps #115

WIP: feat: switch from time-rs to chrono and implement non-daily timesteps #115

Conversation

Batch21 commented Feb 15, 2024

Batch21 commented Feb 16, 2024

jetuk commented Feb 16, 2024

jetuk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Batch21 commented Feb 17, 2024

jetuk commented Feb 17, 2024 via email

jetuk commented Feb 18, 2024

Batch21 commented Feb 21, 2024

jetuk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Batch21 commented Feb 23, 2024

jetuk left a comment

Choose a reason for hiding this comment