Validate epsilon and delta in DPTensorAggregatorBundleFactory::Create…

…Internal. PiperOrigin-RevId: 698817960
JetBrains-Research · Nov 21, 2024 · e23c3e6 · e23c3e6
1 parent 8ec8276
commit e23c3e6
Show file tree

Hide file tree

Showing 3 changed files with 115 additions and 1 deletion.
diff --git a/RELEASE.md b/RELEASE.md
@@ -37,6 +37,11 @@ and this project adheres to
 *   `DPTensorAggregatorBundle`, a wrapper around one or more instances of
     `DPTensorAggregator`, and its factory.
 
+### Changed
+
+*   `DPTensorAggregatorBundleFactory::CreateInternal` now checks validity of the
+    epsilon and delta parameters of its given intrinsic.
+
 ### Removed
 
 *   `tff.types.tensorflow_to_type`, this function is no longer used.

diff --git a/tensorflow_federated/cc/core/impl/aggregation/core/dp_tensor_aggregator_bundle.cc b/tensorflow_federated/cc/core/impl/aggregation/core/dp_tensor_aggregator_bundle.cc
@@ -22,6 +22,7 @@
 
 #include "tensorflow_federated/cc/core/impl/aggregation/base/monitoring.h"
 #include "tensorflow_federated/cc/core/impl/aggregation/core/agg_core.pb.h"
+#include "tensorflow_federated/cc/core/impl/aggregation/core/datatype.h"
 #include "tensorflow_federated/cc/core/impl/aggregation/core/dp_fedsql_constants.h"
 #include "tensorflow_federated/cc/core/impl/aggregation/core/dp_tensor_aggregator.h"
 #include "tensorflow_federated/cc/core/impl/aggregation/core/intrinsic.h"
@@ -98,9 +99,34 @@ DPTensorAggregatorBundleFactory::CreateInternal(
            << "2 parameters, got " << intrinsic.parameters.size();
   }
 
-  // Tests for epsilon and delta parameters will be in the next CL.
+  // Validate epsilon and delta before splitting them.
+  if (internal::GetTypeKind(intrinsic.parameters[kEpsilonIndex].dtype()) !=
+      internal::TypeKind::kNumeric) {
+    return TFF_STATUS(INVALID_ARGUMENT)
+           << "DPTensorAggregatorBundleFactory::CreateInternal: Epsilon must "
+           << "be numerical.";
+  }
   double epsilon = intrinsic.parameters[kEpsilonIndex].AsScalar<double>();
+  if (internal::GetTypeKind(intrinsic.parameters[kDeltaIndex].dtype()) !=
+      internal::TypeKind::kNumeric) {
+    return TFF_STATUS(INVALID_ARGUMENT)
+           << "DPTensorAggregatorBundleFactory::CreateInternal: Delta must "
+           << "be numerical.";
+  }
   double delta = intrinsic.parameters[kDeltaIndex].AsScalar<double>();
+  if (epsilon <= 0) {
+    return TFF_STATUS(INVALID_ARGUMENT) << "DPTensorAggregatorBundleFactory::"
+                                           "CreateInternal: Epsilon must be "
+                                           "positive, but got "
+                                        << epsilon;
+  }
+  if (delta < 0 || delta >= 1) {
+    return TFF_STATUS(INVALID_ARGUMENT)
+           << "DPTensorAggregatorBundleFactory::CreateInternal: Delta must be "
+              "non-negative and less than 1, but got "
+           << delta;
+  }
+
   double epsilon_per_agg =
       (epsilon < kEpsilonThreshold ? epsilon / num_nested_intrinsics
                                    : kEpsilonThreshold);

diff --git a/tensorflow_federated/cc/core/impl/aggregation/core/dp_tensor_aggregator_bundle_test.cc b/tensorflow_federated/cc/core/impl/aggregation/core/dp_tensor_aggregator_bundle_test.cc
@@ -131,6 +131,89 @@ TEST(DPTensorAggregatorBundleTest, CreateWrongNumberOfParameters) {
               HasSubstr("Expected 2 parameters, got 3"));
 }
 
+// Epsilon must be numerical.
+TEST(DPTensorAggregatorBundleTest, CreateWrongTypeOfEpsilon) {
+  Tensor epsilon =
+      Tensor::Create(DT_STRING, {}, CreateTestData<string_view>({"blah"}))
+          .value();
+  Tensor delta =
+      Tensor::Create(DT_DOUBLE, {}, CreateTestData<double>({1e-7})).value();
+  Intrinsic intrinsic = Intrinsic{kDPTensorAggregatorBundleUri, {}, {}, {}, {}};
+  intrinsic.parameters.push_back(std::move(epsilon));
+  intrinsic.parameters.push_back(std::move(delta));
+  intrinsic.nested_intrinsics.push_back(CreateDPQuantileIntrinsic<double>());
+  auto status = CreateTensorAggregator(intrinsic);
+  EXPECT_THAT(status, StatusIs(INVALID_ARGUMENT));
+  EXPECT_THAT(status.status().message(),
+              HasSubstr("Epsilon must be numerical"));
+}
+
+// Epsilon must be positive.
+TEST(DPTensorAggregatorBundleTest, CreateBadEpsilon) {
+  // Exclude 0
+  Intrinsic intrinsic = Intrinsic{
+      kDPTensorAggregatorBundleUri, {}, {}, CreateBundleParameters(0), {}};
+  intrinsic.nested_intrinsics.push_back(CreateDPQuantileIntrinsic<double>());
+  auto status = CreateTensorAggregator(intrinsic);
+  EXPECT_THAT(status, StatusIs(INVALID_ARGUMENT));
+  EXPECT_THAT(status.status().message(),
+              HasSubstr("Epsilon must be positive, but got 0"));
+
+  // Exclude negatives.
+  Intrinsic intrinsic2 = Intrinsic{
+      kDPTensorAggregatorBundleUri, {}, {}, CreateBundleParameters(-1.0), {}};
+  intrinsic2.nested_intrinsics.push_back(CreateDPQuantileIntrinsic<double>());
+  status = CreateTensorAggregator(intrinsic2);
+  EXPECT_THAT(status, StatusIs(INVALID_ARGUMENT));
+  EXPECT_THAT(status.status().message(),
+              HasSubstr("Epsilon must be positive, but got -1"));
+}
+
+// Delta must be numerical.
+TEST(DPTensorAggregatorBundleTest, CreateWrongTypeOfDelta) {
+  Tensor epsilon =
+      Tensor::Create(DT_DOUBLE, {}, CreateTestData<double>({1.0})).value();
+  Tensor delta =
+      Tensor::Create(DT_STRING, {}, CreateTestData<string_view>({"blah"}))
+          .value();
+  Intrinsic intrinsic = Intrinsic{kDPTensorAggregatorBundleUri, {}, {}, {}, {}};
+  intrinsic.parameters.push_back(std::move(epsilon));
+  intrinsic.parameters.push_back(std::move(delta));
+  intrinsic.nested_intrinsics.push_back(CreateDPQuantileIntrinsic<double>());
+  auto status = CreateTensorAggregator(intrinsic);
+  EXPECT_THAT(status, StatusIs(INVALID_ARGUMENT));
+  EXPECT_THAT(status.status().message(), HasSubstr("Delta must be numerical"));
+}
+
+// Delta must be non-negative and less than 1.
+TEST(DPTensorAggregatorBundleTest, CreateBadDelta) {
+  // Exclude larger-than-1.
+  Intrinsic intrinsic = Intrinsic{kDPTensorAggregatorBundleUri,
+                                  {},
+                                  {},
+                                  CreateBundleParameters(1.0, 1.5),
+                                  {}};
+  intrinsic.nested_intrinsics.push_back(CreateDPQuantileIntrinsic<double>());
+  auto status = CreateTensorAggregator(intrinsic);
+  EXPECT_THAT(status, StatusIs(INVALID_ARGUMENT));
+  EXPECT_THAT(status.status().message(),
+              HasSubstr("Delta must be non-negative and less than 1, "
+                        "but got 1.5"));
+
+  // Exclude negatives.
+  Intrinsic intrinsic2 = Intrinsic{kDPTensorAggregatorBundleUri,
+                                   {},
+                                   {},
+                                   CreateBundleParameters(1.0, -0.5),
+                                   {}};
+  intrinsic2.nested_intrinsics.push_back(CreateDPQuantileIntrinsic<double>());
+  status = CreateTensorAggregator(intrinsic2);
+  EXPECT_THAT(status, StatusIs(INVALID_ARGUMENT));
+  EXPECT_THAT(status.status().message(),
+              HasSubstr("Delta must be non-negative and less than 1, "
+                        "but got -0.5"));
+}
+
 // If there is no problem in an intrinsic with 1 quantile aggregator, the
 // bundle should be created successfully. Epsilon and delta should not be split.
 TEST(DPTensorAggregatorBundleTest, CreateSucceeds) {