diff --git a/src/python/BUILD.bazel b/src/python/BUILD.bazel index a0d83669..89238dbc 100644 --- a/src/python/BUILD.bazel +++ b/src/python/BUILD.bazel @@ -100,6 +100,7 @@ pybind_library( srcs = ["s2cell_id_bindings.cc"], deps = [ "//:s2", + "@abseil-cpp//absl/strings", ], ) diff --git a/src/python/README.md b/src/python/README.md index ee62dbbc..9760f2ac 100644 --- a/src/python/README.md +++ b/src/python/README.md @@ -144,7 +144,7 @@ To add bindings for a new class: Use the following sections to organize functions within the bindings files and tests. Secondarily, follow the order in which functions are declared in the C++ headers. 1. **Constructors** - Default constructors and constructors with parameters -1. **Constants** - Class-level constants in upper snake case (e.g., `S2CellId.MAX_LEVEL`, `S2CellId.NUM_FACES`) +1. **Constants** - Class-level constants in upper snake case (e.g., `S2CellId.MAX_LEVEL`, `S2CellId.NUM_FACES`). Accessible as class attributes. 1. **Factory methods** - Static factory methods (e.g., `from_degrees`, `from_radians`, `zero`, `invalid`) 1. **Properties** - Mutable and read-only properties (e.g., coordinate accessors like `x`, `y`, `lo`, `hi`) 1. **Predicates** - Simple boolean state checks (e.g., `is_empty`, `is_valid`, `is_full`) diff --git a/src/python/s2cell_id_bindings.cc b/src/python/s2cell_id_bindings.cc index 6371a40f..b38ebfda 100644 --- a/src/python/s2cell_id_bindings.cc +++ b/src/python/s2cell_id_bindings.cc @@ -60,10 +60,18 @@ void MaybeThrowChildPositionOutOfRange(int position) { } } +void MaybeThrowPositionOutOfRange(uint64_t pos) { + if (pos > S2CellId::kMaxPosition) { + throw py::value_error( + absl::StrCat("pos ", pos, " out of range [0, ", S2CellId::kMaxPosition, + "]")); + } +} + } // namespace void bind_s2cell_id(py::module& m) { - py::class_(m, "S2CellId", + auto cls = py::class_(m, "S2CellId", "A 64-bit unsigned integer that uniquely identifies a cell in the\n" "S2 cell decomposition.\n\n" "See s2/s2cell_id.h for comprehensive documentation.") @@ -88,12 +96,6 @@ void bind_s2cell_id(py::module& m) { py::arg("latlng"), "Construct a leaf cell containing the given S2LatLng.") - // Constants - .def_readonly_static("MAX_LEVEL", &S2CellId::kMaxLevel, - "Maximum cell subdivision level (30)") - .def_readonly_static("NUM_FACES", &S2CellId::kNumFaces, - "Number of cube faces (6)") - // Factory methods .def_static("from_face", [](int face) { MaybeThrowFaceOutOfRange(face); @@ -103,12 +105,13 @@ void bind_s2cell_id(py::module& m) { "Raises ValueError if face is out of range.") .def_static("from_face_pos_level", [](int face, uint64_t pos, int level) { MaybeThrowFaceOutOfRange(face); + MaybeThrowPositionOutOfRange(pos); MaybeThrowLevelOutOfRange(level, 0, S2CellId::kMaxLevel); return S2CellId::FromFacePosLevel(face, pos, level); }, py::arg("face"), py::arg("pos"), py::arg("level"), "Return a cell given its face, Hilbert curve position, and level.\n\n" - "Raises ValueError if face or level is out of range.") + "Raises ValueError if face, pos, or level is out of range.") .def_static("from_token", [](const std::string& token) { S2CellId cell = S2CellId::FromToken(token); if (cell == S2CellId::None()) { @@ -272,4 +275,8 @@ void bind_s2cell_id(py::module& m) { oss << id; return oss.str(); }); + + cls.attr("MAX_LEVEL") = S2CellId::kMaxLevel; + cls.attr("MAX_POSITION") = S2CellId::kMaxPosition; + cls.attr("NUM_FACES") = S2CellId::kNumFaces; } diff --git a/src/python/s2cell_id_test.py b/src/python/s2cell_id_test.py index caf9b3c9..3c31ea2a 100644 --- a/src/python/s2cell_id_test.py +++ b/src/python/s2cell_id_test.py @@ -57,6 +57,10 @@ def test_from_face_pos_level(self): self.assertEqual(cell.level, 0) self.assertEqual(cell.face, 0) + def test_from_face_pos_level_pos_out_of_range_raises(self): + with self.assertRaises(ValueError): + s2.S2CellId.from_face_pos_level(0, s2.S2CellId.MAX_POSITION + 1, 0) + def test_from_token_roundtrip(self): cell = s2.S2CellId.from_face(3) token = cell.to_token() diff --git a/src/s2/s2cell_id.h b/src/s2/s2cell_id.h index 5045eace..e8f5f952 100644 --- a/src/s2/s2cell_id.h +++ b/src/s2/s2cell_id.h @@ -99,6 +99,8 @@ class S2CellId { static constexpr int kMaxLevel = S2::kMaxCellLevel; // Valid levels: 0..kMaxLevel static constexpr int kPosBits = 2 * kMaxLevel + 1; + static constexpr uint64_t kMaxPosition = + (~static_cast(0)) >> kFaceBits; static constexpr int kMaxSize = 1 << kMaxLevel; explicit constexpr S2CellId(uint64_t id) : id_(id) {} diff --git a/src/s2/s2cell_id_test.cc b/src/s2/s2cell_id_test.cc index 9b3fc48b..627335d3 100644 --- a/src/s2/s2cell_id_test.cc +++ b/src/s2/s2cell_id_test.cc @@ -101,6 +101,31 @@ TEST(S2CellId, FromFace) { } } +TEST(S2CellId, MaxPositionIsValid) { + // kMaxPosition is the largest value that fits in the position field. + EXPECT_EQ(S2CellId::kMaxPosition, (~uint64_t{0}) >> S2CellId::kFaceBits); + + // FromFacePosLevel with kMaxPosition produces a valid cell on the correct face. + for (int face = 0; face < S2CellId::kNumFaces; ++face) { + S2CellId id = S2CellId::FromFacePosLevel(face, S2CellId::kMaxPosition, 0); + EXPECT_TRUE(id.is_valid()); + EXPECT_EQ(face, id.face()); + } +} + +TEST(S2CellId, PositionAboveMaxIsInvalid) { + // pos values above kMaxPosition overflow the position field into the face + // bits, producing an invalid cell. + // + // Replicate FromFacePosLevel to avoid its is_valid() DCHECK: + // S2CellId cell((static_cast(face) << kPosBits) + (pos | 1)); + constexpr int face = 5; + constexpr uint64_t kOverflowPos = S2CellId::kMaxPosition + 1; + S2CellId overflow_id( + (uint64_t{face} << S2CellId::kPosBits) + (kOverflowPos | 1)); + EXPECT_FALSE(overflow_id.is_valid()); +} + TEST(S2CellId, ParentChildRelationships) { S2CellId id = S2CellId::FromFacePosLevel(3, 0x12345678, S2CellId::kMaxLevel - 4);