From 404898ce4b5124632845cbfb2b15a07b122a265b Mon Sep 17 00:00:00 2001 From: Toby Schneider Date: Tue, 21 Mar 2023 10:38:45 +1100 Subject: [PATCH 1/2] Fix regression enforcing min/max to be a multiple of the resolution when this wasn't enforced previously for the precision --- src/codecs2/field_codec_default.h | 11 +- src/test/dccl_resolution/test.cpp | 1 + src/test/dccl_resolution/test.proto | 188 ++++++++++++++++------------ 3 files changed, 116 insertions(+), 84 deletions(-) diff --git a/src/codecs2/field_codec_default.h b/src/codecs2/field_codec_default.h index 8163662a..4e2914f0 100644 --- a/src/codecs2/field_codec_default.h +++ b/src/codecs2/field_codec_default.h @@ -129,11 +129,14 @@ namespace dccl // allowable epsilon for min / max to diverge from nearest quantile const double min_max_eps = 1e-10; - - // ensure that max and min are multiples of the resolution chosen - FieldCodecBase::require(std::abs(quantize(min(), resolution()) - min()) < min_max_eps, "(dccl.field).min must be an exact multiple of (dccl.field).resolution"); - FieldCodecBase::require(std::abs(quantize(max(), resolution()) - max()) < min_max_eps, "(dccl.field).max must be an exact multiple of (dccl.field).resolution"); + if(FieldCodecBase::dccl_field_options().has_resolution()) + { + // ensure that max and min are multiples of the resolution chosen + FieldCodecBase::require(std::abs(quantize(min(), resolution()) - min()) < min_max_eps, "(dccl.field).min must be an exact multiple of (dccl.field).resolution"); + FieldCodecBase::require(std::abs(quantize(max(), resolution()) - max()) < min_max_eps, "(dccl.field).max must be an exact multiple of (dccl.field).resolution"); + } + // ensure value fits into double FieldCodecBase::require(std::log2(max() - min()) - std::log2(resolution()) <= std::numeric_limits::digits, "[(dccl.field).max-(dccl.field).min]/(dccl.field).resolution must fit in a double-precision floating point value. Please increase min, decrease max, or decrease precision."); diff --git a/src/test/dccl_resolution/test.cpp b/src/test/dccl_resolution/test.cpp index c4f72b94..ba8acc17 100644 --- a/src/test/dccl_resolution/test.cpp +++ b/src/test/dccl_resolution/test.cpp @@ -102,6 +102,7 @@ int main(int argc, char* argv[]) msg_in.set_u3(10.2); msg_in.set_u4(5.6); msg_in.set_u5(1.95); + msg_in.set_u6(100); std::string encoded; codec.encode(&encoded, msg_in); diff --git a/src/test/dccl_resolution/test.proto b/src/test/dccl_resolution/test.proto index 48c2136b..8a4560c7 100644 --- a/src/test/dccl_resolution/test.proto +++ b/src/test/dccl_resolution/test.proto @@ -1,106 +1,134 @@ @PROTOBUF_SYNTAX_VERSION@ + import "dccl/option_extensions.proto"; package dccl.test; message NumericMsg { - option (dccl.msg).id = 10; - option (dccl.msg).max_bytes = 32; - option (dccl.msg).codec_version = 4; - - optional double a = 1 [(dccl.field).max = 180, - (dccl.field).min = -180, - (dccl.field).precision = 12, - (dccl.field).in_head=true]; - - optional double b = 2 [(dccl.field).max = 18, - (dccl.field).min = -18, - (dccl.field).resolution = 0.0001]; - - // max is 2^64 rounded to 1e5 - required uint64 u1 = 3 [(dccl.field).max = 18446744073709500000, - (dccl.field).min = 0, - (dccl.field).precision = -5]; - - // max is 2^64 rounded to 1e5 - required uint64 u2 = 4 [(dccl.field).max = 18446744073709500000, - (dccl.field).min = 0, - (dccl.field).resolution = 100000]; - - // resolution != 10^N - required double u3 = 5 [(dccl.field).max = 15.5, - (dccl.field).min = 5.5, - (dccl.field).resolution = 0.5]; - - // resolution default - required double u4 = 6 [(dccl.field).max = 20.0, - (dccl.field).min = 0.0]; - - // weird resolution - required double u5 = 7 [(dccl.field).max = 3.6, - (dccl.field).min = 1.44, - (dccl.field).resolution = 0.12]; - - + option (dccl.msg).id = 10; + option (dccl.msg).max_bytes = 32; + option (dccl.msg).codec_version = 4; + + optional double a = 1 [ + (dccl.field).max = 180, + (dccl.field).min = -180, + (dccl.field).precision = 12, + (dccl.field).in_head = true + ]; + + optional double b = 2 [ + (dccl.field).max = 18, + (dccl.field).min = -18, + (dccl.field).resolution = 0.0001 + ]; + + // max is 2^64 rounded to 1e5 + required uint64 u1 = 3 [ + (dccl.field).max = 18446744073709500000, + (dccl.field).min = 0, + (dccl.field).precision = -5 + ]; + + // max is 2^64 rounded to 1e5 + required uint64 u2 = 4 [ + (dccl.field).max = 18446744073709500000, + (dccl.field).min = 0, + (dccl.field).resolution = 100000 + ]; + + // resolution != 10^N + required double u3 = 5 [ + (dccl.field).max = 15.5, + (dccl.field).min = 5.5, + (dccl.field).resolution = 0.5 + ]; + + // resolution default + required double u4 = 6 [(dccl.field).max = 20.0, (dccl.field).min = 0.0]; + + // weird resolution + required double u5 = 7 [ + (dccl.field).max = 3.6, + (dccl.field).min = 1.44, + (dccl.field).resolution = 0.12 + ]; + + // old precision field did not require min and max to be a multiple of resolution + required int32 u6 = 8 [ + (dccl.field).min = 0, + (dccl.field).max = 16383, + (dccl.field).precision = -2 + ]; } -message NegativeResolutionNumericMsg // Invalid resolution +message NegativeResolutionNumericMsg // Invalid resolution { - option (dccl.msg).id = 10; - option (dccl.msg).max_bytes = 32; - option (dccl.msg).codec_version = 4; - - optional double a = 1 [(dccl.field).min = -20, - (dccl.field).max = 20, - (dccl.field).resolution = -0.5]; - - optional int32 b = 2 [(dccl.field).min = -500000, - (dccl.field).max = 500000, - (dccl.field).precision = -3]; + option (dccl.msg).id = 10; + option (dccl.msg).max_bytes = 32; + option (dccl.msg).codec_version = 4; + + optional double a = 1 [ + (dccl.field).min = -20, + (dccl.field).max = 20, + (dccl.field).resolution = -0.5 + ]; + + optional int32 b = 2 [ + (dccl.field).min = -500000, + (dccl.field).max = 500000, + (dccl.field).precision = -3 + ]; } - message BothResolutionAndPrecisionSetNumericMsg { - option (dccl.msg).id = 11; - option (dccl.msg).max_bytes = 32; - option (dccl.msg).codec_version = 4; - - optional double a = 1 [(dccl.field).max = 180, - (dccl.field).min = -180, - (dccl.field).precision = 1, - (dccl.field).resolution = 0.1]; + option (dccl.msg).id = 11; + option (dccl.msg).max_bytes = 32; + option (dccl.msg).codec_version = 4; + + optional double a = 1 [ + (dccl.field).max = 180, + (dccl.field).min = -180, + (dccl.field).precision = 1, + (dccl.field).resolution = 0.1 + ]; } message TooBigNumericMsg { - option (dccl.msg).id = 11; - option (dccl.msg).max_bytes = 32; - option (dccl.msg).codec_version = 4; - - optional double a = 1 [(dccl.field).max = 180, - (dccl.field).min = -180, - (dccl.field).resolution = 1e-15]; + option (dccl.msg).id = 11; + option (dccl.msg).max_bytes = 32; + option (dccl.msg).codec_version = 4; + + optional double a = 1 [ + (dccl.field).max = 180, + (dccl.field).min = -180, + (dccl.field).resolution = 1e-15 + ]; } message MinNotMultipleOfResolution { - option (dccl.msg).id = 11; - option (dccl.msg).max_bytes = 32; - option (dccl.msg).codec_version = 4; - - required double a = 1 [(dccl.field).max = 3.6, - (dccl.field).min = 1.5, - (dccl.field).resolution = 0.2]; + option (dccl.msg).id = 11; + option (dccl.msg).max_bytes = 32; + option (dccl.msg).codec_version = 4; + + required double a = 1 [ + (dccl.field).max = 3.6, + (dccl.field).min = 1.5, + (dccl.field).resolution = 0.2 + ]; } message MaxNotMultipleOfResolution { - option (dccl.msg).id = 11; - option (dccl.msg).max_bytes = 32; - option (dccl.msg).codec_version = 4; - - required double a = 1 [(dccl.field).max = 3.5, - (dccl.field).min = 1.4, - (dccl.field).resolution = 0.2]; + option (dccl.msg).id = 11; + option (dccl.msg).max_bytes = 32; + option (dccl.msg).codec_version = 4; + + required double a = 1 [ + (dccl.field).max = 3.5, + (dccl.field).min = 1.4, + (dccl.field).resolution = 0.2 + ]; } From 1135df37e2978a4b2739efbac8c4a87c52f9e88d Mon Sep 17 00:00:00 2001 From: Toby Schneider Date: Tue, 21 Mar 2023 11:02:30 +1100 Subject: [PATCH 2/2] Demote require to warning for min/max multiple check when using precision --- src/codecs2/field_codec_default.h | 16 +++++++++++++--- src/test/dccl_resolution/test.cpp | 2 +- src/test/dccl_resolution/test.proto | 2 +- 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/codecs2/field_codec_default.h b/src/codecs2/field_codec_default.h index 4e2914f0..5127f097 100644 --- a/src/codecs2/field_codec_default.h +++ b/src/codecs2/field_codec_default.h @@ -129,12 +129,22 @@ namespace dccl // allowable epsilon for min / max to diverge from nearest quantile const double min_max_eps = 1e-10; - + bool min_multiple_of_res = std::abs(quantize(min(), resolution()) - min()) < min_max_eps; + bool max_multiple_of_res = std::abs(quantize(max(), resolution()) - max()) < min_max_eps; if(FieldCodecBase::dccl_field_options().has_resolution()) { // ensure that max and min are multiples of the resolution chosen - FieldCodecBase::require(std::abs(quantize(min(), resolution()) - min()) < min_max_eps, "(dccl.field).min must be an exact multiple of (dccl.field).resolution"); - FieldCodecBase::require(std::abs(quantize(max(), resolution()) - max()) < min_max_eps, "(dccl.field).max must be an exact multiple of (dccl.field).resolution"); + FieldCodecBase::require(min_multiple_of_res, "(dccl.field).min must be an exact multiple of (dccl.field).resolution"); + FieldCodecBase::require(max_multiple_of_res, "(dccl.field).max must be an exact multiple of (dccl.field).resolution"); + } + else + { + auto res = resolution(); + // this was previously allowed so we will only give a warning not throw an exception + if(!min_multiple_of_res) + dccl::dlog.is(dccl::logger::WARN, dccl::logger::GENERAL) && dccl::dlog << "Warning: (dccl.field).min should be an exact multiple of 10^(-(dccl.field).precision), i.e. " << res << ": " << this->this_field()->DebugString() << std::endl; + if(!max_multiple_of_res) + dccl::dlog.is(dccl::logger::WARN, dccl::logger::GENERAL) && dccl::dlog << "Warning: (dccl.field).max should be an exact multiple of 10^(-(dccl.field).precision), i.e. " << res << ": " << this->this_field()->DebugString() << std::endl; } // ensure value fits into double diff --git a/src/test/dccl_resolution/test.cpp b/src/test/dccl_resolution/test.cpp index ba8acc17..8064e72c 100644 --- a/src/test/dccl_resolution/test.cpp +++ b/src/test/dccl_resolution/test.cpp @@ -102,7 +102,7 @@ int main(int argc, char* argv[]) msg_in.set_u3(10.2); msg_in.set_u4(5.6); msg_in.set_u5(1.95); - msg_in.set_u6(100); + msg_in.set_u6(25500); std::string encoded; codec.encode(&encoded, msg_in); diff --git a/src/test/dccl_resolution/test.proto b/src/test/dccl_resolution/test.proto index 8a4560c7..49f0936f 100644 --- a/src/test/dccl_resolution/test.proto +++ b/src/test/dccl_resolution/test.proto @@ -56,7 +56,7 @@ message NumericMsg // old precision field did not require min and max to be a multiple of resolution required int32 u6 = 8 [ (dccl.field).min = 0, - (dccl.field).max = 16383, + (dccl.field).max = 25599, (dccl.field).precision = -2 ]; }