Repository: alexhsamuel/ora (language: python)

Issue:
round to nearest when formatting



Discussion / hints:

Fix patch:
diff --git a/cxx/include/ora/format.hh b/cxx/include/ora/format.hh
index 39843fd..facc649 100644
--- a/cxx/include/ora/format.hh
+++ b/cxx/include/ora/format.hh
@@ -173,28 +173,9 @@ public:
 
 protected:
 
-  /*
-   * Includes all the various parts that can be used for formatting.
-   */
-  struct Parts
-  {
-    FullDate      date            = {};
-    bool          have_date       = false;
-    HmsDaytime    daytime         = {};
-    bool          have_daytime    = false;
-    TimeZoneParts time_zone       = {};
-    bool          have_time_zone  = false;
-  };
-
-  std::string
-  format(
-    Parts const& parts)
-    const
-  {
-    StringBuilder sb;
-    format(sb, parts);
-    return sb.str();
-  }
+  std::string format(Datenum) const;
+  std::string format(HmsDaytime const&) const;
+  std::string format(LocalDatenumDaytick const&) const;
 
   std::string const&
   get_invalid_pad()
@@ -220,18 +201,12 @@ private:
   {
     if (width_ == -1) {
       // Find the length of a formatted string.
-      auto const parts = Parts{
-        .date = FullDate{
-          {YEAR_MIN, ORDINAL_MIN},
-          {YEAR_MIN, MONTH_MIN, DAY_MIN},
-          {YEAR_MIN, WEEK_MIN, WEEKDAY_MIN}},
-        .have_date = true,
-        .daytime = HmsDaytime{0, 0, 0},
-        .have_daytime = true,
-        .time_zone = TimeZoneParts{0, "", false},
-        .have_time_zone = true,
+      static auto const MIN_TIME = LocalDatenumDaytick{
+        DATENUM_MIN,
+        DAYTICK_MIN,
+        TimeZoneParts{0, "", false},
       };
-      width_ = (int) format(parts).length();
+      width_ = (int) format(MIN_TIME).length();
 
       // Truncate or pad the invalid and missing strings.
       invalid_pad_ = pad_trunc(invalid_, width_, ' ');
@@ -239,8 +214,6 @@ private:
     }
   }
 
-  void format(StringBuilder&, Parts const&) const;
-
   std::string const pattern_;
   std::string const invalid_;
   std::string const missing_;
@@ -298,10 +271,7 @@ public:
     HmsDaytime const& hms)
     const
   {
-    return format(Parts{
-      .date = {}, .have_date = false,
-      .daytime = hms, .have_daytime = true
-    });
+    return format(hms);
   }
 
   template<class TRAITS>
@@ -314,7 +284,7 @@ public:
     return
         daytime.is_invalid() ? (fixed ? get_invalid_pad() : get_invalid())
       : daytime.is_missing() ? (fixed ? get_missing_pad() : get_missing())
-      : operator()(get_hms(daytime));
+      : format(get_hms(daytime));
   }
 
 };
@@ -394,14 +364,7 @@ public:
     LocalDatenumDaytick const& ldd)
     const
   {
-    return format(Parts{
-      .date           = datenum_to_full_date(ldd.datenum),
-      .have_date      = true,
-      .daytime        = daytick_to_hms(ldd.daytick),
-      .have_daytime   = true,
-      .time_zone      = ldd.time_zone,
-      .have_time_zone = true,
-    });
+    return format(ldd);
   }
 
   /*
@@ -576,10 +539,7 @@ public:
     return
         date.is_invalid() ? (fixed ? get_invalid_pad() : get_invalid())
       : date.is_missing() ? (fixed ? get_missing_pad() : get_missing())
-      : format(Parts{
-          .date = datenum_to_full_date(date.get_datenum()),
-          .have_date = true,
-        });
+      : format(date.get_datenum());
   }
 
 };
diff --git a/cxx/src/format.cc b/cxx/src/format.cc
index b8a0f78..8ce6500 100644
--- a/cxx/src/format.cc
+++ b/cxx/src/format.cc
@@ -344,69 +344,109 @@ format_time(
 
 namespace _impl {
 
-void
-Format::format(
-  StringBuilder& sb,
-  Parts const& parts)
-  const
+inline size_t
+find_next_escape(
+  std::string const& pattern,
+  size_t pos,
+  StringBuilder& sb)
 {
-  size_t pos = 0;
   while (true) {
-    // Find the next escape character.
-    size_t const next = pattern_.find('%', pos);
+    size_t const next = pattern.find('%', pos);
     if (next == std::string::npos) {
       // No next escape.  Copy the rest of the pattern, and we're done.
-      sb << pattern_.substr(pos);
-      break;
+      sb << pattern.substr(pos);
+      return std::string::npos;
     }
     else if (next > pos)
       // Copy from the pattern until the next escape.
-      sb << pattern_.substr(pos, next - pos);
+      sb << pattern.substr(pos, next - pos);
     // Skip over the escape character.
     pos = next + 1;
+    // Literal %?
+    if (pos == pattern.length())
+      throw ValueError("unterminated escape in pattern");
+    else if (pattern[pos] == '%') {
+      sb << '%';
+      pos++;
+      continue;
+    }
+    else
+      return pos;
+  }
+}
+
+
+std::string
+Format::format(
+  Datenum datenum)
+  const
+{
+  auto const date = datenum_to_full_date(datenum);
+
+  StringBuilder sb;
+  for (size_t pos = find_next_escape(pattern_, 0, sb);
+       pos != std::string::npos;
+       pos = find_next_escape(pattern_, pos, sb)) {
+    // Set up state for the escape sequence.
+    Modifiers mods;
+    // Scan characters in the escape sequence.
+    while (parse_modifiers(pattern_, pos, mods))
+      ;
+    if (!format_date(pattern_, pos, sb, mods, date))
+      throw TimeFormatError(
+        std::string("unknown date escape '") + pattern_[pos] + "'");
+  }
+  return sb.str();
+}
 
+
+std::string
+Format::format(
+  HmsDaytime const& hms)
+  const
+{
+  StringBuilder sb;
+  for (size_t pos = find_next_escape(pattern_, 0, sb);
+       pos != std::string::npos;
+       pos = find_next_escape(pattern_, pos, sb)) {
     // Set up state for the escape sequence.
     Modifiers mods;
+    // Scan characters in the escape sequence.
+    while (parse_modifiers(pattern_, pos, mods))
+      ;
+    if (!format_daytime(pattern_, pos, sb, mods, hms))
+      throw TimeFormatError(
+        std::string("unknown daytime escape '") + pattern_[pos] + "'");
+  }
+  return sb.str();
+}
+
 
+std::string
+Format::format(
+  LocalDatenumDaytick const& ldd)
+  const
+{
+  auto const date = datenum_to_full_date(ldd.datenum);
+  auto const hms = daytick_to_hms(ldd.daytick);
+
+  StringBuilder sb;
+  for (size_t pos = find_next_escape(pattern_, 0, sb);
+       pos != std::string::npos;
+       pos = find_next_escape(pattern_, pos, sb)) {
+    // Set up state for the escape sequence.
+    Modifiers mods;
     // Scan characters in the escape sequence.
-    for (bool done = false; ! done; ) {
-      if (pos == pattern_.length())
-        throw ValueError("unterminated escape in pattern");
-
-      // Literal '%' escape.
-      if (pattern_[pos] == '%') {
-        sb << '%';
-        pos++;
-        break;
-      }
-
-      // Handle modifiers.
-      if (parse_modifiers(pattern_, pos, mods))
-        continue;
-
-      // Handle escape codes for date components.
-      if (   parts.have_date
-          && format_date(pattern_, pos, sb, mods, parts.date))
-        break;
-      if (   parts.have_daytime
-          && format_daytime(pattern_, pos, sb, mods, parts.daytime))
-        break;
-      if (   parts.have_time_zone
-          && format_time_zone(pattern_, pos, sb, mods, parts.time_zone))
-        break;
-      if (   parts.have_date
-          && parts.have_daytime
-          && parts.have_time_zone
-          && format_time(
-            pattern_, pos, sb, mods,
-            parts.date, parts.daytime, parts.time_zone))
-        break;
-
-      // If we made it this far, it's not a valid character.
+    while (parse_modifiers(pattern_, pos, mods))
+      ;
+    if (   !format_time(pattern_, pos, sb, mods, date, hms, ldd.time_zone)
+        && !format_date(pattern_, pos, sb, mods, date)
+        && !format_daytime(pattern_, pos, sb, mods, hms)
+        && !format_time_zone(pattern_, pos, sb, mods, ldd.time_zone))
       throw TimeFormatError(
-        std::string("unknown escape '") + pattern_[pos] + "'");
-    }
+        std::string("unknown time escape '") + pattern_[pos] + "'");
   }
+  return sb.str();
 }
 
 
diff --git a/python/ora/ext/py_time.hh b/python/ora/ext/py_time.hh
index 803534b..408968c 100644
--- a/python/ora/ext/py_time.hh
+++ b/python/ora/ext/py_time.hh
@@ -926,6 +926,31 @@ convert_to_time(
   if (time.first)
     return time.second;
 
+  if (obj->IsInstance(np::Descr::from(NPY_DATETIME)->typeobj)) {
+    // Get the exact dtype and its tick denominator.
+    auto descr = PyArray_DescrFromScalar(obj);
+    auto const den = np::get_datetime64_denominator(descr);
+    // Get the epoch tick value.
+    int64_t val;
+    PyArray_ScalarAsCtype(obj, &val);
+    if (val == np::DATETIME64_NAT)
+      return TIME::INVALID;
+    // Convert to an offset.
+    auto const offset
+      = round_div<int128_t>((int128_t) val * TIME::DENOMINATOR, den)
+        + (long(DATENUM_UNIX_EPOCH) - TIME::BASE)
+          * SECS_PER_DAY * TIME::DENOMINATOR;
+
+    // Check bounds before (possibly) narrowing to offset.
+    if (
+            offset < TIME::Traits::min
+         || offset > TIME::Traits::max)
+      throw py::OverflowError(
+        "time out of range: '"s + obj->Repr()->as_utf8() + "'");
+
+    return ora::time::nex::from_offset<TIME>(offset);
+  }
+
   if (Unicode::Check(obj)) {
     auto const str = static_cast<Unicode*>(obj)->as_utf8_string();
     if (str == "MIN")
@@ -973,31 +998,6 @@ convert_to_time(
       Exception::Clear();
   }
 
-  if (obj->IsInstance(np::Descr::from(NPY_DATETIME)->typeobj)) {
-    // Get the exact dtype and its tick denominator.
-    auto descr = PyArray_DescrFromScalar(obj);
-    auto const den = np::get_datetime64_denominator(descr);
-    // Get the epoch tick value.
-    int64_t val;
-    PyArray_ScalarAsCtype(obj, &val);
-    if (val == np::DATETIME64_NAT)
-      return TIME::INVALID;
-    // Convert to an offset.
-    auto const offset
-      = round_div<int128_t>((int128_t) val * TIME::DENOMINATOR, den)
-        + (long(DATENUM_UNIX_EPOCH) - TIME::BASE)
-          * SECS_PER_DAY * TIME::DENOMINATOR;
-
-    // Check bounds before (possibly) narrowing to offset.
-    if (
-            offset < TIME::Traits::min
-         || offset > TIME::Traits::max)
-      throw py::OverflowError(
-        "time out of range: '"s + obj->Repr()->as_utf8() + "'");
-
-    return ora::time::nex::from_offset<TIME>(offset);
-  }
-
   throw py::TypeError("can't convert to a time: "s + *obj->Repr());
 }
 
diff --git a/python/ora/ext/py_time_fmt.cc b/python/ora/ext/py_time_fmt.cc
index 109d783..87d6304 100644
--- a/python/ora/ext/py_time_fmt.cc
+++ b/python/ora/ext/py_time_fmt.cc
@@ -61,15 +61,15 @@ ref<Object> tp_call(PyTimeFmt* self, Tuple* args, Dict* kw_args)
   Object* arg;
   Arg::ParseTupleAndKeywords(args, kw_args, "O", arg_names, &arg);
 
-  // Use exact ns resolution here to avoid FP approximation issues with Python's
-  // µs times or NumPy's and Pandas's ns times.
+  // Use high resolution time here so we can round correctly.
   auto const time = convert_to_time<NsTime>(arg);
   if (time.is_invalid())
     return Unicode::from(self->invalid_);
   else if (time.is_missing())
     return Unicode::from(self->missing_);
   else {
-    auto const ldd = to_local_datenum_daytick(time, *UTC);
+    // Round to nearest instead of truncate.
+    auto const ldd = to_local_datenum_daytick(time + self->round_step_, *UTC);
     StringBuilder sb;
     time::format_iso_time(
       sb, datenum_to_ymd(ldd.datenum), daytick_to_hms(ldd.daytick),
diff --git a/python/ora/ext/py_time_fmt.hh b/python/ora/ext/py_time_fmt.hh
index c90b489..3f7fe85 100644
--- a/python/ora/ext/py_time_fmt.hh
+++ b/python/ora/ext/py_time_fmt.hh
@@ -1,5 +1,6 @@
 #pragma once
 
+#include <cmath>
 #include <Python.h>
 
 #include "py.hh"
@@ -23,6 +24,7 @@ public:
     string  const& invalid  ="INVALID",
     string  const& missing  ="MISSING")
   : precision_(precision),
+    round_step_(nextafter(pow(10, -std::max(precision, 0)) / 2, 0)),
     invalid_(palide(invalid, get_width(), "", " ", 1, PAD_POS_LEFT_JUSTIFY)),
     missing_(palide(missing, get_width(), "", " ", 1, PAD_POS_LEFT_JUSTIFY))
   {
@@ -31,6 +33,7 @@ public:
   long          get_width() const { return 26 + precision_; }
 
   int const     precision_;
+  double const  round_step_;
   string const  invalid_;
   string const  missing_;
 


Test patch:
diff --git a/python/ora/test/test_fmt.py b/python/ora/test/test_fmt.py
new file mode 100644
index 0000000..326e88e
--- /dev/null
+++ b/python/ora/test/test_fmt.py
@@ -0,0 +1,95 @@
+import numpy as np
+import pytest
+
+from   ora import Date, Date16, Time128, DateFmt, TimeFmt, TIME_TYPES
+
+#-------------------------------------------------------------------------------
+
+def test_date_fmt():
+    fmt = DateFmt()
+    assert fmt((1, 1, 1)) == "0001-01-01"
+    assert fmt("2022-08-22") == "2022-08-22"
+    assert fmt(Date("1969-12-31")) == "1969-12-31"
+    assert fmt(Date16("1970-01-01")) == "1970-01-01"
+    assert fmt(Date.INVALID) == "INVALID   "
+    assert fmt(Date16.MISSING) == "MISSING   "
+
+
+def test_date_fmt_invalid():
+    fmt = DateFmt(invalid="!!!!!!!!", missing="????????????")
+    assert fmt(Date.MISSING) == "??????????"
+    assert fmt(Date.INVALID) == "!!!!!!!!  "
+
+
+def test_time_fmt():
+    fmt = TimeFmt(-1)
+    assert fmt("2022-08-23T00:07:52.123Z") == "2022-08-23T00:07:52+00:00"
+    assert fmt("2022-08-23T00:07:52.789Z") == "2022-08-23T00:07:53+00:00"
+
+    fmt = TimeFmt(1)
+    assert fmt("2022-08-23T00:07:52.123Z") == "2022-08-23T00:07:52.1+00:00"
+    assert fmt("2022-08-23T00:07:52.789Z") == "2022-08-23T00:07:52.8+00:00"
+
+    fmt = TimeFmt(4)
+    assert fmt("2022-08-23T00:07:52.123Z") == "2022-08-23T00:07:52.1230+00:00"
+    assert fmt("2022-08-23T00:07:52.789Z") == "2022-08-23T00:07:52.7890+00:00"
+
+    fmt = TimeFmt(8)
+    assert fmt("2022-08-23T00:07:52.123Z") == "2022-08-23T00:07:52.12300000+00:00"
+    assert fmt("2022-08-23T00:07:52.789Z") == "2022-08-23T00:07:52.78900000+00:00"
+
+    fmt = TimeFmt(9)
+    assert fmt("2022-08-23T00:07:52.123Z") == "2022-08-23T00:07:52.123000000+00:00"
+    assert fmt("2022-08-23T00:07:52.789Z") == "2022-08-23T00:07:52.789000000+00:00"
+    assert fmt("2022-08-23T00:07:52.123456789Z") == "2022-08-23T00:07:52.123456789+00:00"
+
+
+def test_time_fmt_datetime64():
+    t = np.datetime64("2022-09-01T01:23:45.345678901", "ns")
+    assert TimeFmt(-1)(t) == "2022-09-01T01:23:45+00:00"
+    assert TimeFmt( 0)(t) == "2022-09-01T01:23:45.+00:00"
+    assert TimeFmt( 1)(t) == "2022-09-01T01:23:45.3+00:00"
+    assert TimeFmt( 2)(t) == "2022-09-01T01:23:45.35+00:00"
+    assert TimeFmt( 3)(t) == "2022-09-01T01:23:45.346+00:00"
+    assert TimeFmt( 4)(t) == "2022-09-01T01:23:45.3457+00:00"
+    assert TimeFmt( 5)(t) == "2022-09-01T01:23:45.34568+00:00"
+    assert TimeFmt( 6)(t) == "2022-09-01T01:23:45.345679+00:00"
+    assert TimeFmt( 7)(t) == "2022-09-01T01:23:45.3456789+00:00"
+    assert TimeFmt( 8)(t) == "2022-09-01T01:23:45.34567890+00:00"
+    assert TimeFmt( 9)(t) == "2022-09-01T01:23:45.345678901+00:00"
+    assert TimeFmt(10)(t) == "2022-09-01T01:23:45.3456789010+00:00"
+
+
+def test_time_fmt_128():
+    t = Time128("2022-09-01T12:34:56.789012345678+00:00")
+    assert TimeFmt(-1)(t) == "2022-09-01T12:34:57+00:00"
+    assert TimeFmt( 0)(t) == "2022-09-01T12:34:57.+00:00"
+    assert TimeFmt( 1)(t) == "2022-09-01T12:34:56.8+00:00"
+    assert TimeFmt( 2)(t) == "2022-09-01T12:34:56.79+00:00"
+    assert TimeFmt( 3)(t) == "2022-09-01T12:34:56.789+00:00"
+    assert TimeFmt( 4)(t) == "2022-09-01T12:34:56.7890+00:00"
+    assert TimeFmt( 5)(t) == "2022-09-01T12:34:56.78901+00:00"
+    assert TimeFmt( 6)(t) == "2022-09-01T12:34:56.789012+00:00"
+    assert TimeFmt( 7)(t) == "2022-09-01T12:34:56.7890123+00:00"
+    assert TimeFmt( 8)(t) == "2022-09-01T12:34:56.78901235+00:00"
+    assert TimeFmt( 9)(t) == "2022-09-01T12:34:56.789012346+00:00"
+
+
+# Currently doesn't work because we use NsTime in the implementation.
+@pytest.mark.xfail
+def test_time_fmt_128hi():
+    t = Time128("2022-09-01T12:34:56.789012345678+00:00")
+    assert TimeFmt(10)(t) == "2022-09-01T12:34:56.7890123457+00:00"
+
+
+@pytest.mark.parametrize("Time", TIME_TYPES)
+def test_time_fmt_invalid(Time):
+    fmt = TimeFmt(-1, invalid="!!!!!!!!", missing="????????????")
+    assert fmt(Time.MISSING) == "????????????             "
+    assert fmt(Time.INVALID) == "!!!!!!!!                 "
+
+    fmt = TimeFmt(2, invalid="!!!!!!!!", missing="????????????")
+    assert fmt(Time.MISSING) == "????????????                "
+    assert fmt(Time.INVALID) == "!!!!!!!!                    "
+
+