Skip to content

Commit ea915b3

Browse files
Dobiasdclaude
andcommitted
Address review findings
C++ correctness: - Fix UB: MappedRowMajorMatrixXf used Eigen::Aligned over std::vector data which is not SIMD-aligned. Switch the typedef to const + Unaligned and drop the const_cast at every call site. - IntegerLookup: replace abs(v) UB at INT64_MIN with a single-OOV-bucket assertion (Keras hashes OOV across multiple buckets via FarmHash, which fdeep does not currently replicate, so num_oov_indices > 1 is rejected). - EinsumDense: assert rhs has at least 2 chars before slicing off the leading batch dim. - GRU: replace silent zero-fallback for malformed bias with an explicit size assertion that respects reset_after. - GroupedQueryAttention: assert num_key_value_heads > 0 before the modulo. - AutoContrast removed from the passthrough dispatch — it transforms deterministically at inference and isn't actually a no-op. Performance: - EinsumDense: precompute summed_strides instead of recomputing the inner stride loop per output element. Style: - Make member fields const in newly-added layer classes for consistency with the rest of the codebase. - show_bidirectional_layer: validate merge_mode and reject forward_layer.go_backwards explicitly. Test coverage (added in get_test_model_recurrent): - LSTM with return_state=True (h, c outputs) and unit_forget_bias=False. - GroupedQueryAttention with num_query_heads == num_key_value_heads (= MultiHeadAttention) and with distinct K/V sequence lengths. - ConvLSTM1D with dilation_rate, two ConvLSTM2D layers chained. - GroupNormalization with groups=1 (= LayerNorm) and groups=n_features (= InstanceNorm). - Discretization, Masking, RandomBrightness, RandomFlip, RandomCrop now exercised end-to-end. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent b824708 commit ea915b3

16 files changed

Lines changed: 143 additions & 86 deletions

include/fdeep/common.hpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ namespace internal {
8080
using RowMajorMatrixXf = Eigen::Matrix<float_type, Eigen::Dynamic, Eigen::Dynamic, Eigen::RowMajor>;
8181
using ArrayXf = Eigen::Array<float_type, Eigen::Dynamic, Eigen::Dynamic>;
8282
using ArrayXf1D = Eigen::Array<float_type, Eigen::Dynamic, 1>;
83-
using MappedRowMajorMatrixXf = Eigen::Map<RowMajorMatrixXf, Eigen::Aligned>;
83+
using MappedRowMajorMatrixXf = Eigen::Map<const RowMajorMatrixXf, Eigen::Unaligned>;
8484

8585
inline float_type tanh_typed(float_type x)
8686
{

include/fdeep/import_model.hpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1822,8 +1822,10 @@ namespace internal {
18221822
{ "RandomTranslation", create_identity_layer },
18231823
{ "RandomWidth", create_identity_layer },
18241824
{ "RandomZoom", create_identity_layer },
1825+
// AutoContrast is intentionally NOT registered as a passthrough:
1826+
// it applies a deterministic per-image min/max stretch even at
1827+
// inference. Supporting it would require a real implementation.
18251828
{ "AugMix", create_identity_layer },
1826-
{ "AutoContrast", create_identity_layer },
18271829
{ "CutMix", create_identity_layer },
18281830
{ "Equalization", create_identity_layer },
18291831
{ "MaxNumBoundingBoxes", create_identity_layer },

include/fdeep/layers/adaptive_pooling_3d_layer.hpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -35,10 +35,10 @@ namespace internal {
3535
}
3636

3737
protected:
38-
std::size_t out_d4_;
39-
std::size_t out_h_;
40-
std::size_t out_w_;
41-
adaptive_pooling_kind kind_;
38+
const std::size_t out_d4_;
39+
const std::size_t out_h_;
40+
const std::size_t out_w_;
41+
const adaptive_pooling_kind kind_;
4242

4343
static std::size_t adapt_start(std::size_t i, std::size_t in_size, std::size_t out_size)
4444
{

include/fdeep/layers/conv_lstm_2d_layer.hpp

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -45,14 +45,14 @@ namespace internal {
4545
}
4646

4747
protected:
48-
std::size_t units_;
49-
std::size_t rank_;
50-
bool return_sequences_;
51-
bool return_state_;
52-
std::string activation_;
53-
std::string recurrent_activation_;
54-
conv_2d_layer input_conv_;
55-
conv_2d_layer recurrent_conv_;
48+
const std::size_t units_;
49+
const std::size_t rank_;
50+
const bool return_sequences_;
51+
const bool return_state_;
52+
const std::string activation_;
53+
const std::string recurrent_activation_;
54+
const conv_2d_layer input_conv_;
55+
const conv_2d_layer recurrent_conv_;
5656

5757
static tensor extract_timestep(const tensor& input, std::size_t t,
5858
std::size_t rank)

include/fdeep/layers/conv_lstm_3d_layer.hpp

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -43,13 +43,13 @@ namespace internal {
4343
}
4444

4545
protected:
46-
std::size_t units_;
47-
bool return_sequences_;
48-
bool return_state_;
49-
std::string activation_;
50-
std::string recurrent_activation_;
51-
conv_3d_layer input_conv_;
52-
conv_3d_layer recurrent_conv_;
46+
const std::size_t units_;
47+
const bool return_sequences_;
48+
const bool return_state_;
49+
const std::string activation_;
50+
const std::string recurrent_activation_;
51+
const conv_3d_layer input_conv_;
52+
const conv_3d_layer recurrent_conv_;
5353

5454
static tensor extract_timestep(const tensor& input, std::size_t t)
5555
{

include/fdeep/layers/discretization_layer.hpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ namespace internal {
2525
}
2626

2727
protected:
28-
std::vector<float_type> boundaries_;
28+
const std::vector<float_type> boundaries_;
2929

3030
tensors apply_impl(const tensors& inputs) const override
3131
{

include/fdeep/layers/einsum_dense_layer.hpp

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -37,16 +37,16 @@ namespace internal {
3737
}
3838

3939
protected:
40-
std::string equation_;
41-
std::vector<int> full_output_shape_;
42-
std::string bias_axes_;
43-
tensor kernel_;
44-
tensor bias_;
40+
const std::string equation_;
41+
const std::vector<int> full_output_shape_;
42+
const std::string bias_axes_;
43+
const tensor kernel_;
44+
const tensor bias_;
4545

46-
std::string lhs_;
47-
std::string rhs_kernel_;
48-
std::string rhs_;
49-
std::string summed_;
46+
const std::string lhs_;
47+
const std::string rhs_kernel_;
48+
const std::string rhs_;
49+
const std::string summed_;
5050

5151
static std::string parse_lhs(const std::string& eq)
5252
{
@@ -157,6 +157,7 @@ namespace internal {
157157
const auto lhs_strides = compute_strides(lhs_sizes);
158158
const auto kernel_strides = compute_strides(kernel_sizes);
159159
const auto rhs_strides = compute_strides(rhs_sizes);
160+
const auto summed_strides = compute_strides(summed_sizes);
160161

161162
std::size_t out_volume = 1;
162163
for (auto sz : rhs_sizes)
@@ -189,11 +190,8 @@ namespace internal {
189190
{
190191
std::size_t rem = s_idx;
191192
for (std::size_t i = 0; i < summed_.size(); ++i) {
192-
std::size_t stride = 1;
193-
for (std::size_t j = i + 1; j < summed_.size(); ++j)
194-
stride *= summed_sizes[j];
195-
pos[summed_[i]] = rem / stride;
196-
rem %= stride;
193+
pos[summed_[i]] = rem / summed_strides[i];
194+
rem %= summed_strides[i];
197195
}
198196
}
199197
std::size_t lhs_offset = 0;
@@ -231,6 +229,8 @@ namespace internal {
231229

232230
// Build the output tensor shape from rhs_sizes minus the leading
233231
// batch char (its size in fdeep is 1, dropped).
232+
assertion(rhs_sizes.size() >= 2,
233+
"EinsumDense output equation must have at least a batch char and one feature char.");
234234
std::vector<std::size_t> out_dims_no_batch(rhs_sizes.begin() + 1, rhs_sizes.end());
235235
return { tensor(create_tensor_shape_from_dims(out_dims_no_batch), std::move(out)) };
236236
}

include/fdeep/layers/group_normalization_layer.hpp

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -33,11 +33,11 @@ namespace internal {
3333
}
3434

3535
protected:
36-
std::size_t groups_;
37-
int axis_;
38-
float_type epsilon_;
39-
float_vec beta_;
40-
float_vec gamma_;
36+
const std::size_t groups_;
37+
const int axis_;
38+
const float_type epsilon_;
39+
const float_vec beta_;
40+
const float_vec gamma_;
4141

4242
tensors apply_impl(const tensors& inputs) const override
4343
{

include/fdeep/layers/group_query_attention_layer.hpp

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -29,17 +29,19 @@ namespace internal {
2929
, use_gate_(use_gate)
3030
, weights_(weights)
3131
{
32+
assertion(num_kv_heads_ > 0,
33+
"num_key_value_heads must be > 0.");
3234
assertion(num_query_heads_ % num_kv_heads_ == 0,
3335
"num_query_heads must be divisible by num_key_value_heads.");
3436
}
3537

3638
protected:
37-
std::size_t head_dim_;
38-
std::size_t num_query_heads_;
39-
std::size_t num_kv_heads_;
40-
bool use_bias_;
41-
bool use_gate_;
42-
std::vector<tensor> weights_;
39+
const std::size_t head_dim_;
40+
const std::size_t num_query_heads_;
41+
const std::size_t num_kv_heads_;
42+
const bool use_bias_;
43+
const bool use_gate_;
44+
const std::vector<tensor> weights_;
4345

4446
// Weight order in Keras: Q, K, [Gate], V, Out, with bias right after each kernel.
4547
std::size_t weight_idx(std::size_t projection_idx) const

include/fdeep/layers/integer_lookup_layer.hpp

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -30,15 +30,20 @@ namespace internal {
3030
, vocab_offset_((has_mask_token ? 1 : 0) + num_oov_indices)
3131
, vocab_index_()
3232
{
33+
// Keras hashes OOV inputs across multiple OOV buckets via FarmHash,
34+
// which fdeep does not currently replicate. Restrict to a single
35+
// OOV bucket (the common case) to keep behavior bit-identical.
36+
assertion(num_oov_indices_ <= 1,
37+
"IntegerLookup with num_oov_indices > 1 is not supported.");
3338
for (std::size_t i = 0; i < vocabulary.size(); ++i)
3439
vocab_index_[vocabulary[i]] = i;
3540
}
3641

3742
protected:
38-
bool has_mask_token_;
39-
std::int64_t mask_token_;
40-
std::size_t num_oov_indices_;
41-
std::size_t vocab_offset_;
43+
const bool has_mask_token_;
44+
const std::int64_t mask_token_;
45+
const std::size_t num_oov_indices_;
46+
const std::size_t vocab_offset_;
4247
std::unordered_map<std::int64_t, std::size_t> vocab_index_;
4348

4449
tensors apply_impl(const tensors& inputs) const override
@@ -56,13 +61,9 @@ namespace internal {
5661
if (it != vocab_index_.end()) {
5762
out[i] = static_cast<float_type>(vocab_offset_ + it->second);
5863
} else {
59-
// OOV bucket: floormod hash. With num_oov_indices=1, always 0
60-
// (offset by mask slot if present).
61-
const std::size_t oov_bucket = num_oov_indices_ == 0
62-
? 0
63-
: (static_cast<std::size_t>(v < 0 ? -v : v) % num_oov_indices_);
64-
out[i] = static_cast<float_type>(
65-
(has_mask_token_ ? 1 : 0) + oov_bucket);
64+
// num_oov_indices_ <= 1 (asserted in ctor): OOV maps to 0,
65+
// shifted by 1 if a mask token occupies index 0.
66+
out[i] = static_cast<float_type>(has_mask_token_ ? 1 : 0);
6667
}
6768
}
6869
return { tensor(input.shape(), std::move(out)) };

0 commit comments

Comments
 (0)