diff --git a/src/odc/Comparator.cc b/src/odc/Comparator.cc index 2f2641fe..44a12610 100644 --- a/src/odc/Comparator.cc +++ b/src/odc/Comparator.cc @@ -8,7 +8,6 @@ * does it submit to any jurisdiction. */ -#include "eckit/config/Resource.h" #include "eckit/exception/Exceptions.h" #include "eckit/filesystem/PathName.h" #include "eckit/log/Log.h" @@ -41,8 +40,7 @@ class ValuesDifferent : public Exception { namespace odc { -Comparator::Comparator(bool skipTestingHaveMissing) : - skipTestingHaveMissing_(skipTestingHaveMissing), nRow_(0), NaN_isOK_(Resource("$odc_NAN_IS_OK", false)) {} +Comparator::Comparator(bool skipTestingHaveMissing) : skipTestingHaveMissing_(skipTestingHaveMissing), nRow_(0) {} void Comparator::compare(const PathName& p1, const PathName& p2) { @@ -168,12 +166,11 @@ void Comparator::compare(int nCols, const double* data1, const double* data2, co case INTEGER: case BITFIELD: case DOUBLE: - if (!(same(*pdata1, *pdata2) || (NaN_isOK_ && (::isnan(*pdata1) && ::isnan(*pdata2))))) + if (!same(*pdata1, *pdata2)) raiseNotEqual(column, *pdata1, *pdata2); break; case REAL: - if (!(same(float(*pdata1), float(*pdata2)) || - (NaN_isOK_ && (::isnan(*pdata1) && ::isnan(*pdata2))))) + if (!same(float(*pdata1), float(*pdata2))) raiseNotEqual(column, *pdata1, *pdata2); break; case IGNORE: diff --git a/src/odc/Comparator.h b/src/odc/Comparator.h index 51ec3460..950491f0 100644 --- a/src/odc/Comparator.h +++ b/src/odc/Comparator.h @@ -77,7 +77,9 @@ class Comparator { return relativeError; } - inline static int same(double A, double B) { return err(A, B) < maxRelativeError; } + inline static bool same(double A, double B) { + return err(A, B) < maxRelativeError || (std::isnan(A) && std::isnan(B)); + } void raiseNotEqual(const core::Column&, double, double); @@ -85,7 +87,6 @@ class Comparator { bool skipTestingHaveMissing_; long nRow_; - bool NaN_isOK_; }; template diff --git a/src/odc/codec/Integer.h b/src/odc/codec/Integer.h index 683f1721..f2226d53 100644 --- a/src/odc/codec/Integer.h +++ b/src/odc/codec/Integer.h @@ -11,6 +11,11 @@ #ifndef odc_core_codec_Integer_H #define odc_core_codec_Integer_H +#include + +#include "eckit/exception/Exceptions.h" + +#include "odc/ODBAPISettings.h" #include "odc/core/Codec.h" /// @note We have some strange behaviour in here. In particular, we support BOTH decoding @@ -36,7 +41,9 @@ class BaseCodecInteger : public core::DataStreamCodec { public: // methods BaseCodecInteger(api::ColumnType type, const std::string& name, double minmaxmissing = odc::MDI::integerMDI()) : - core::DataStreamCodec(name, type), castedMissingValue_(static_cast(minmaxmissing)) { + core::DataStreamCodec(name, type), + castedMissingValue_(static_cast(minmaxmissing)), + integersAsDoubles_(ODBAPISettings::instance().integersAsDoubles()) { this->min_ = minmaxmissing; this->max_ = minmaxmissing; @@ -65,6 +72,9 @@ class BaseCodecInteger : public core::DataStreamCodec { void gatherStats(const double& v) override { static_assert(sizeof(ValueType) == sizeof(v), "unsafe casting check"); + if (integersAsDoubles_ && std::isnan(v)) { + throw eckit::UserError("NaN is not a valid value in INTEGER/BITFIELD column '" + this->name() + "'"); + } const ValueType& val(reinterpret_cast(v)); core::Codec::gatherStats(val); } @@ -75,6 +85,8 @@ class BaseCodecInteger : public core::DataStreamCodec { /// directly where needed is to work around a Cray 8.7 compiler bug, where /// where the punned version gets optimised out ValueType castedMissingValue_; + + const bool integersAsDoubles_; }; diff --git a/src/odc/core/Codec.cc b/src/odc/core/Codec.cc index 303e5505..12c969bb 100644 --- a/src/odc/core/Codec.cc +++ b/src/odc/core/Codec.cc @@ -10,6 +10,8 @@ #include "odc/core/Codec.h" +#include + #include "eckit/exception/Exceptions.h" #include "odc/core/CodecFactory.h" @@ -98,6 +100,8 @@ void Codec::missingValue(double v) { } void Codec::gatherStats(const double& v) { + if (std::isnan(v)) + return; if (v == missingValue_) { hasMissing_ = 1; } diff --git a/tests/core/CMakeLists.txt b/tests/core/CMakeLists.txt index 1a07dbc6..b6e3facc 100644 --- a/tests/core/CMakeLists.txt +++ b/tests/core/CMakeLists.txt @@ -25,6 +25,7 @@ list( APPEND _core_odc_tests test_text_reader test_table_iterator test_initial_missing + test_comparator ) foreach( _test ${_core_odc_tests} ) diff --git a/tests/core/test_codecs_end_to_end.cc b/tests/core/test_codecs_end_to_end.cc index 2a244ec9..b1c48bdd 100644 --- a/tests/core/test_codecs_end_to_end.cc +++ b/tests/core/test_codecs_end_to_end.cc @@ -8,13 +8,19 @@ * does it submit to any jurisdiction. */ +#include +#include + #include "eckit/io/Buffer.h" #include "eckit/io/DataHandle.h" #include "eckit/io/MemoryHandle.h" #include "eckit/testing/Test.h" +#include "odc/MDI.h" #include "odc/Reader.h" #include "odc/Writer.h" +#include "odc/api/ColumnType.h" +#include "odc/api/Odb.h" #include "odc/codec/Integer.h" #include "odc/codec/String.h" #include "odc/core/MetaData.h" @@ -304,6 +310,121 @@ CASE("Missing values are encoded and decoded correctly") { } +CASE("NaN before real values preserves correct min/max in column metadata") { + + const double NaN = std::numeric_limits::quiet_NaN(); + + eckit::Buffer buf(4096); + eckit::MemoryHandle writeDH(buf); + + { + odc::Writer<> oda(writeDH); + odc::Writer<>::iterator writer = oda.begin(); + + writer->setNumberOfColumns(1); + writer->setColumn(0, "col0", odc::api::DOUBLE); + writer->writeHeader(); + + (*writer)[0] = NaN; + ++writer; + (*writer)[0] = 1.1; + ++writer; + (*writer)[0] = 3.3; + ++writer; + } + + eckit::MemoryHandle dh(buf.data(), static_cast(writeDH.position())); + dh.openForRead(); + odc::Reader oda(dh); + odc::Reader::iterator it = oda.begin(); + + EXPECT_EQUAL(it->columns()[0]->min(), 1.1); + EXPECT_EQUAL(it->columns()[0]->max(), 3.3); +} + + +CASE("NaN round-trips as NaN in REAL/DOUBLE columns") { + + const double NaN = std::numeric_limits::quiet_NaN(); + + for (auto type : {odc::api::DOUBLE, odc::api::REAL}) { + + eckit::Buffer buf(4096); + eckit::MemoryHandle writeDH(buf); + + { + odc::Writer<> oda(writeDH); + odc::Writer<>::iterator writer = oda.begin(); + + writer->setNumberOfColumns(1); + writer->setColumn(0, "col0", type); + writer->writeHeader(); + + (*writer)[0] = 1.0; + ++writer; + (*writer)[0] = NaN; + ++writer; + (*writer)[0] = 3.0; + ++writer; + } + + eckit::MemoryHandle dh(buf.data(), static_cast(writeDH.position())); + dh.openForRead(); + odc::Reader oda(dh); + + odc::Reader::iterator it = oda.begin(); + EXPECT_EQUAL((*it)[0], 1.0); + ++it; + EXPECT(std::isnan((*it)[0])); + ++it; + EXPECT_EQUAL((*it)[0], 3.0); + ++it; + EXPECT_EQUAL(it, oda.end()); + } +} + +CASE("NaN is rejected in INTEGER/BITFIELD columns") { + + const double NaN = std::numeric_limits::quiet_NaN(); + + for (auto type : {odc::api::INTEGER, odc::api::BITFIELD}) { + + eckit::Buffer buf(4096); + eckit::MemoryHandle writeDH(buf); + + odc::Writer<> oda(writeDH); + odc::Writer<>::iterator writer = oda.begin(); + + writer->setNumberOfColumns(1); + writer->setColumn(0, "col0", type); + writer->writeHeader(); + + (*writer)[0] = 1.0; + ++writer; + (*writer)[0] = NaN; + EXPECT_THROWS_AS(++writer, eckit::UserError); + } +} + + +CASE("api::encode rejects NaN in INTEGER/BITFIELD columns") { + + const double NaN = std::numeric_limits::quiet_NaN(); + std::vector values{1.0, NaN, 3.0}; + + for (auto type : {odc::api::INTEGER, odc::api::BITFIELD}) { + + eckit::Buffer buf(4096); + eckit::MemoryHandle writeDH(buf); + writeDH.openForWrite(0); + + std::vector columns{{"col0", type, sizeof(double), {}}}; + std::vector strides{{values.data(), values.size(), sizeof(double), sizeof(double)}}; + EXPECT_THROWS_AS(odc::api::encode(writeDH, columns, strides), eckit::UserError); + } +} + + // ------------------------------------------------------------------------------------------------------ int main(int argc, char* argv[]) { diff --git a/tests/core/test_comparator.cc b/tests/core/test_comparator.cc new file mode 100644 index 00000000..c59423d9 --- /dev/null +++ b/tests/core/test_comparator.cc @@ -0,0 +1,45 @@ +/* + * (C) Copyright 1996-2012 ECMWF. + * + * This software is licensed under the terms of the Apache Licence Version 2.0 + * which can be obtained at http://www.apache.org/licenses/LICENSE-2.0. + * In applying this licence, ECMWF does not waive the privileges and immunities + * granted to it by virtue of its status as an intergovernmental organisation nor + * does it submit to any jurisdiction. + */ + +#include + +#include "eckit/testing/Test.h" + +#include "odc/Comparator.h" + +using namespace eckit::testing; + +// ------------------------------------------------------------------------------------------------------ + +CASE("Comparator treats two NaNs as equal") { + const double NaN = std::numeric_limits::quiet_NaN(); + + EXPECT(odc::Comparator::same(NaN, NaN)); + EXPECT(!odc::Comparator::same(NaN, 1.0)); + EXPECT(!odc::Comparator::same(1.0, NaN)); +} + +CASE("Comparator treats identical values as equal") { + EXPECT(odc::Comparator::same(0.0, 0.0)); + EXPECT(odc::Comparator::same(1.0, 1.0)); + EXPECT(odc::Comparator::same(-1.0, -1.0)); +} + +CASE("Comparator treats different values as unequal") { + EXPECT(!odc::Comparator::same(0.0, 1.0)); + EXPECT(!odc::Comparator::same(1.0, 2.0)); + EXPECT(!odc::Comparator::same(1.0, -1.0)); +} + +// ------------------------------------------------------------------------------------------------------ + +int main(int argc, char* argv[]) { + return run_tests(argc, argv); +}