feat: Move to jiff datetime library. - #744
Conversation
e8d5263 to
30853b9
Compare
|
I have put a separate commit on here to bump |
Batch21
left a comment
Batch21
left a comment
There was a problem hiding this comment.
A few queries but runs ok for me.
| let mut timesteps: Vec<Timestep> = Vec::new(); | ||
| let mut current = self.start; | ||
| while current <= self.end { | ||
| let next = current + span; |
There was a problem hiding this comment.
I think next needs to be
let next = self.start + span * (timesteps.len() as i64 + 1);If the timestep is a monthly it will currently go 02-28 -> 03-28 instead of 02-28 -> 03-31
There was a problem hiding this comment.
Is this any different to polars::time::date_range?
>>> pl.date_range(date(2000, 2, 28), date(2000, 8, 31), '1mo', eager=True).alias("date")
shape: (7,)
Series: 'date' [date]
[
2000-02-28
2000-03-28
2000-04-28
2000-05-28
2000-06-28
2000-07-28
2000-08-28
]
There was a problem hiding this comment.
I think the issue is with 29th–31st start days, where the date will get clamped to the 28th for all months after the first Feb.
| @@ -330,44 +338,28 @@ impl TimeDomainBuilder { | |||
| let duration = polars::time::Duration::parse(frequency); | |||
|
|
|||
There was a problem hiding this comment.
should we check here that the duration is not zero or negative? Previously this would have be done by the polars range function.
| } | ||
| VirtualStorageReset::Annual(annual) => pywr_core::virtual_storage::VirtualStorageReset::DayOfYear { | ||
| day: annual.day, | ||
| month: annual.month, |
There was a problem hiding this comment.
Now that the core month values are i8 not chrono Months should we have some validation here that the values are in the correct range? Same for the other month fields below
| fn eq(&self, other: &Span) -> bool { | ||
| self.0 | ||
| .compare(SpanCompare::from(other).days_are_24_hours()) | ||
| .expect("Span comparison failed!") |
There was a problem hiding this comment.
are the expects ok in these methods? one way to avoid them would be to switch to using SignedDuration instead of Span as the innner type but I don't know if that removes some required functinality?
There was a problem hiding this comment.
If we wrap SignedDuration in PywrDuration then we lose the knowledge it was "1 month" or "2 days". It just becomes a (possibly) different duration for each timestep. This is possibly OK as mainly just need to know the length of each one anyway. However, it means that each timestep is no longer equal. Currently we have a PartialEq implementation that is used to validate each timestep is equal in the series. I am not sure that is really required though.
9ff62c6 to
f80642b
Compare
Chrono is now soft-deprecated. Much of the Rust ecosystem is switching to jiff. These changes move to Jiff for all but the interface with polars. Polars is still using chrono, and we cannot drop the chrono dependency until polars does. There are some minor integer type changes. The biggest change is the removal of the `Date` enum in favour of just using `jiff::civil::DateTime` directly. However, this changes the roundtrip deserlisation/serialisation behaviour losing the information on whether the user provided a simple date or a date-time. We now Use jiff::SignedDuration internally for the `PywrDuration` this is consistent with the use of 24-hour days. The `TimeDomain` is updated to remove the assumption that it has a fixed duration. Instead a helper method can return the fixed duration *if* there is one. The conversion tests have been updated to reflect this change.
f80642b to
8d4af80
Compare
2 participants
Chrono is now soft-deprecated. Much of the Rust ecosystem is switching to jiff. These changes move to Jiff for all but the interface with polars. Polars is still using chrono, and we cannot drop the chrono dependency until polars does.
There are some minor integer type changes. The biggest change is the removal of the
Dateenum in favour of just usingjiff::civil::DateTimedirectly. However, this changes the roundtrip deserlisation/serialisation behaviour losing the information on whether the user provided a simple date or a date-time.The conversion tests currently fail because they are not updated to reflect this change.