diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index d5a31b2..1e3dfd6 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -10,9 +10,11 @@ permissions: jobs: build: - # fastfields-lib has no standalone tests (correctness is gated by - # fastfields-cpu-lib). This is a build/link check: `make all` compiles - # libfastfields.so and also builds + installs libfastfields-cpu.so. + # Op correctness is gated by fastfields-cpu-lib. Here: a build/link check + # (`make all` compiles libfastfields.so and also builds + installs + # libfastfields-cpu.so), plus `make test` for the standalone header-only + # tests covering the argument validation that lives in this repo only + # (require_same_device, require_splinc_bound). runs-on: ubuntu-latest steps: - uses: actions/checkout@v5 @@ -47,6 +49,9 @@ jobs: # recursive `cpu` sub-make forward $(CXX) straight into clang++, so no # Makefile change is needed. run: make -C . all CXX="ccache clang++" + # Header-only, no link against libfastfields.so -- runs in seconds. + - name: Run standalone tests + run: make -C . test CXX="ccache clang++" - name: Show ccache stats if: always() run: ccache -s diff --git a/CLAUDE.md b/CLAUDE.md index 00037c7..468044d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -47,11 +47,14 @@ be refactored onto a new tensor library later. ## Build & test ``` make -C . all CXX=clang++ # builds libfastfields.so; also builds+installs libfastfields-cpu.so +make -C . test CXX=clang++ # compiles+runs the standalone tests/test_*.cpp ``` -No standalone tests at this level — correctness is gated by -`fastfields-cpu-lib`'s test suite. CUDA is compile/link-only (no GPU in CI). -Submodule symlinks must exist (`lib/cpu -> cpu-lib`, `cpu-lib/impl -> cpu-impl`, -`cpu-impl/kernels -> kernels`). +**Op** correctness is gated by `fastfields-cpu-lib`'s test suite. `tests/` here +holds only header-only tests for the argument validation that lives in this +repo and nowhere below it (`checks.h`'s `require_same_device`, `splinc.h`'s +`require_splinc_bound`); they link nothing and run in seconds. CUDA is +compile/link-only (no GPU in CI). Submodule symlinks must exist +(`lib/cpu -> cpu-lib`, `cpu-lib/impl -> cpu-impl`, `cpu-impl/kernels -> kernels`). ## Conventions & caveats - **C++11**, clang-style Makefile flags, object rule needs `-fPIC`. Add a module diff --git a/Makefile b/Makefile index c79cd9d..f45432b 100644 --- a/Makefile +++ b/Makefile @@ -120,9 +120,9 @@ endif all: lib -clean: clean-lib clean-obj clean-cpu +clean: clean-lib clean-obj clean-test clean-cpu -.PHONY: all clean +.PHONY: all clean test ######################################################################## # Build directory @@ -141,9 +141,33 @@ clean-obj: clean-lib: $(DEL) $(BUILDDIR)/*.$(SOSUF) +clean-test: + $(DEL) $(TESTBINS) + clean-cpu: $(MAKE) -C cpu clean +######################################################################## +# Tests +######################################################################## + +# Standalone, header-only tests: each tests/test_.cpp includes only the hub +# headers it exercises (checks.h, splinc.h, ...), so it compiles and runs on its +# own without linking libfastfields.so. Op correctness stays gated by +# fastfields-cpu-lib's suite -- these cover the argument validation that lives +# in this repo and nowhere below it. + +TESTS = $(patsubst tests/test_%.cpp,%,$(wildcard tests/test_*.cpp)) +TESTBINS = $(addprefix $(BUILDDIR)/test_,$(TESTS)) + +test: $(TESTBINS) + $(call verb, "Running tests...") + @ for t in $(TESTBINS); do echo "--- $$t"; $$t || exit 1; done + $(call verb, "Running tests: done.") + +$(BUILDDIR)/test_%: tests/test_%.cpp | $(BUILDDIR) + $(CXX) $(CXXFLAGS) $(TESTFLAGS) $(INCLUDES) -I. -o $@ $< + ######################################################################## # Library ######################################################################## diff --git a/splinc.cpp b/splinc.cpp index 8b65961..8e05ba7 100644 --- a/splinc.cpp +++ b/splinc.cpp @@ -18,6 +18,11 @@ void spline_coeff( int8_t bound , intptr_t stream ) { + // Reject boundary conditions the prefilter does not implement. Must run + // before dispatch: neither backend validates `bound`, and both alias the + // unimplemented ones onto the DCT1 recursion (fastfields-lib#65). + require_splinc_bound(spline, bound); + #ifdef FF_WITH_CUDA if (IS_CUDA(inp_out)) return FF_CUDA::spline_coeff(inp_out, spline, bound, stream); diff --git a/splinc.h b/splinc.h index 58c30b7..a89ef1c 100644 --- a/splinc.h +++ b/splinc.h @@ -2,6 +2,8 @@ #define FF_LIB_SPLINC #include "dlpack.h" #include +#include +#include #include "defines.h" #ifndef FF_LIB_BOUND_SPLINE_T @@ -39,6 +41,57 @@ FF_NAMESPACE_END(FF) FF_NAMESPACE_BEGIN(FF) +/** + * @brief Assert that `bound` is a boundary condition the prefilter implements. + * + * The prefilter's recursion only has initial/final conditions derived for + * whole-point mirroring (`DCT1`), half-point mirroring (`DCT2`, and + * `Replicate`, which shares them) and circulant wrapping (`DFT`) -- the same + * four that `jitfields.splinc.checkbound` allows. Every other `bound_t` value + * has no implementation of its own and falls through to the `DCT1` recursion + * in the kernels, so a caller asking for e.g. `Zero` silently gets whole-point + * mirroring instead: results bit-identical to `DCT1`, with no diagnostic + * (fastfields-lib#65). + * + * Reject those up front rather than answering a boundary condition nobody + * asked for. This is the hub's single dispatch point for `spline_coeff`, so + * the check covers the CPU and CUDA backends and every downstream binding. + * + * Orders 0 and 1 are exempt: the prefilter is the identity there, never runs + * the recursion, and so never touches the boundary. Orders outside 0..7 are + * left alone too, so the backend's "unsupported spline order" stays the error + * the caller sees. + * + * @param spline Spline order (see spline_t) + * @param bound Boundary condition (see bound_t) + */ +inline void require_splinc_bound(int8_t spline, int8_t bound) +{ + // Identity orders (0/1) and out-of-range orders: nothing to validate here. + if (spline < spline_t::Quadratic || spline > spline_t::SeventhOrder) return; + + switch (bound) { + case bound_t::Replicate: + case bound_t::DCT1: + case bound_t::DCT2: + case bound_t::DFT: return; + default: break; + } + + const char * name; + switch (bound) { + case bound_t::Zero: name = "zero"; break; + case bound_t::DST1: name = "dst1"; break; + case bound_t::DST2: name = "dst2"; break; + case bound_t::NoCheck: name = "nocheck"; break; + default: name = "unknown"; break; + } + throw std::invalid_argument( + std::string("fastfields: `spline_coeff` is only implemented for bounds " + "(dct1, dct2, dft, replicate) but got: ") + + name); +} + /** * @brief In-place spline coefficient prefilter along the last dimension. * @@ -48,7 +101,9 @@ FF_NAMESPACE_BEGIN(FF) * * @param inp_out Input/Output tensor in DLTensor format (float32/float64) * @param spline Spline order (orders 0/1 are no-ops) - * @param bound Boundary condition + * @param bound Boundary condition. Only dct1/dct2/dft/replicate are + * implemented; anything else throws `std::invalid_argument` + * (see require_splinc_bound). Ignored for orders 0/1. * @param stream Cuda stream on which to operate */ void spline_coeff( diff --git a/tests/test_splinc_bound.cpp b/tests/test_splinc_bound.cpp new file mode 100644 index 0000000..f33f150 --- /dev/null +++ b/tests/test_splinc_bound.cpp @@ -0,0 +1,129 @@ +// Standalone unit test for the spline_coeff bound guard (fastfields-lib #65). +// +// `bound=zero` (and dst1/dst2/nocheck) has no prefilter recursion of its own: +// the kernels fall through to the DCT1 initial/final conditions, so the caller +// silently got whole-point mirroring -- results bit-identical to dct1 -- for a +// boundary condition they never asked for. ff::require_splinc_bound rejects +// those up front, mirroring jitfields' `splinc.checkbound`. +// +// Header-only: it exercises the guard itself, not the dispatch, so no library +// link is needed. Build it via `make test`, or directly from the repo root: +// clang++ -std=c++11 -I. tests/test_splinc_bound.cpp -o /tmp/t && /tmp/t + +#include +#include +#include +#include +#include "../splinc.h" + +static int failures = 0; + +#define CHECK(cond, msg) \ + do { \ + if (!(cond)) { \ + std::printf("FAIL: %s\n", msg); \ + ++failures; \ + } \ + else { \ + std::printf("ok: %s\n", msg); \ + } \ + } while (0) + +// Returns the message if require_splinc_bound threw, or an empty string. +static std::string threw_with(int8_t spline, int8_t bound) +{ + try { + ff::require_splinc_bound(spline, bound); + } + catch (const std::invalid_argument & e) { + return std::string(e.what()); + } + return std::string(); +} + +static bool contains(const std::string & hay, const char * needle) +{ + return hay.find(needle) != std::string::npos; +} + +int main() +{ + // The four orders that actually run the recursion, plus the two identity + // orders and an out-of-range one. + const int8_t real_orders[6] = { + ff::spline_t::Quadratic, ff::spline_t::Cubic, + ff::spline_t::FourthOrder, ff::spline_t::FifthOrder, + ff::spline_t::SixthOrder, ff::spline_t::SeventhOrder}; + + // 1. The implemented bounds must never throw, at any real order. + { + const int8_t ok_bounds[4] = {ff::bound_t::DCT1, ff::bound_t::DCT2, + ff::bound_t::DFT, ff::bound_t::Replicate}; + bool any_threw = false; + for (int o = 0; o < 6; ++o) + for (int b = 0; b < 4; ++b) + if (!threw_with(real_orders[o], ok_bounds[b]).empty()) + any_threw = true; + CHECK(!any_threw, + "dct1/dct2/dft/replicate accepted at every order 2..7"); + } + + // 2. bound=zero must throw at every order that runs the prefilter. + // This is the regression: it used to be silently aliased to dct1. + { + bool all_threw = true; + for (int o = 0; o < 6; ++o) + if (threw_with(real_orders[o], ff::bound_t::Zero).empty()) + all_threw = false; + CHECK(all_threw, "bound=zero throws at every order 2..7"); + } + + // 3. The message names the offending bound and the implemented set. + { + std::string msg = threw_with(ff::spline_t::Cubic, ff::bound_t::Zero); + CHECK(contains(msg, "zero"), + "error message names the rejected bound (zero)"); + CHECK(contains(msg, "dct1") && contains(msg, "dct2") && + contains(msg, "dft") && contains(msg, "replicate"), + "error message lists the implemented bounds"); + } + + // 4. The other unimplemented bounds throw too (dst1/dst2 already raised + // from the torch binding; the guard makes that uniform across backends). + { + CHECK(contains(threw_with(ff::spline_t::Cubic, ff::bound_t::DST1), + "dst1"), + "bound=dst1 throws"); + CHECK(contains(threw_with(ff::spline_t::Cubic, ff::bound_t::DST2), + "dst2"), + "bound=dst2 throws"); + CHECK(contains(threw_with(ff::spline_t::Cubic, ff::bound_t::NoCheck), + "nocheck"), + "bound=nocheck throws"); + } + + // 5. Orders 0/1 are the identity -- the bound is never consulted, so even + // an unimplemented one must be accepted (matches jitfields' checkbound). + { + bool threw = + !threw_with(ff::spline_t::Nearest, ff::bound_t::Zero).empty() || + !threw_with(ff::spline_t::Linear, ff::bound_t::Zero).empty(); + CHECK(!threw, + "orders 0/1 accept any bound (prefilter is the identity)"); + } + + // 6. An out-of-range order is not this guard's business: it must stay + // silent so the backend's "unsupported spline order" is what surfaces. + { + bool threw = !threw_with((int8_t)8, ff::bound_t::Zero).empty() || + !threw_with((int8_t)-1, ff::bound_t::Zero).empty(); + CHECK(!threw, "out-of-range orders defer to the backend's order error"); + } + + if (failures) { + std::printf("\n%d check(s) FAILED\n", failures); + return 1; + } + std::printf("\nAll checks passed.\n"); + return 0; +}