Skip to content

Commit 568321e

Browse files
Dobiasdclaude
andcommitted
Address second-round review findings
Latent bugs fixed: - EinsumDense bias ordering: Keras stores the bias variable with dims in rhs char order, not bias_axes-string order. Reorder bias_axes_ chars to match rhs_ before computing strides so the right element is read. - EinsumDense output_shape index: Keras's output_shape excludes the batch dim, so indexing rhs[i] against full_output_shape_[i] was off by one for output-only chars. Prepend a -1 placeholder for the batch char in the C++ creator to keep positional indexing consistent. Defensive guards: - IntegerLookup now requires num_oov_indices == 1 (was <= 1, which silently accepted strict mode that Keras would have raised on). - Add n_units > 0 assertion to lstm_impl, gru_impl, simple_rnn_impl. - Add groups > 0 / group_volume > 0 assertions to GroupNormalization. - Add out_size > 0 assertion to AdaptivePooling adapt_range. Style: - Make IntegerLookup's vocab_index_ const and populate via a helper called from the member init list, matching the rest of the new layer files. Test coverage: - LSTM had return_state=True but GRU and SimpleRNN didn't — added. - GroupedQueryAttention with num_query_heads == num_key_value_heads now also exercises distinct K/V sequence lengths (cross-attention shape). - IntegerLookup is now reached by an end-to-end model (chained after Discretization to get an integer-valued tensor). - Several more Random* augmentation passthroughs (RandomContrast, RandomRotation, RandomTranslation, RandomZoom, RandomHue, RandomSaturation, RandomSharpness) now go through the test pipeline. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent baf492d commit 568321e

7 files changed

Lines changed: 60 additions & 15 deletions

File tree

include/fdeep/import_model.hpp

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -825,7 +825,10 @@ namespace internal {
825825
const std::string bias_axes = config["bias_axes"].is_null()
826826
? std::string("")
827827
: std::string(config["bias_axes"]);
828+
// Keras's output_shape excludes the batch dimension. Prepend -1 for
829+
// the batch char so the layer can index it positionally against rhs.
828830
std::vector<int> output_shape;
831+
output_shape.push_back(-1);
829832
for (const auto& dim : config["output_shape"])
830833
output_shape.push_back(dim.is_null() ? -1 : static_cast<int>(dim));
831834

include/fdeep/layers/adaptive_pooling_3d_layer.hpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ namespace internal {
5353
{
5454
if (in_size == 1)
5555
return { 0, 1 };
56+
assertion(out_size > 0, "AdaptivePooling output_size must be > 0.");
5657
const auto start = static_cast<std::size_t>(std::floor(
5758
static_cast<double>(i * in_size) / static_cast<double>(out_size)));
5859
const auto end = static_cast<std::size_t>(std::ceil(

include/fdeep/layers/einsum_dense_layer.hpp

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -195,20 +195,29 @@ namespace internal {
195195
}
196196

197197
// Adds bias broadcast over the configured bias_axes (a subset of rhs).
198+
// Keras stores the bias variable with its dims in rhs char order, not
199+
// in the literal bias_axes string order (e.g. bias_axes='ed' on
200+
// rhs='abde' still produces a (d, e)-shaped bias). Reorder the chars
201+
// before computing strides so we read the right element.
198202
void add_bias(float_vec& out,
199203
const std::map<char, std::size_t>& char_size,
200204
const std::vector<std::size_t>& rhs_strides) const
201205
{
202206
if (bias_axes_.empty())
203207
return;
208+
std::string bias_chars;
209+
for (char c : rhs_)
210+
if (bias_axes_.find(c) != std::string::npos)
211+
bias_chars.push_back(c);
212+
204213
const auto& bias_vals = *bias_.as_vector();
205-
const auto bias_sizes = sizes_for_chars(bias_axes_, char_size);
214+
const auto bias_sizes = sizes_for_chars(bias_chars, char_size);
206215
const auto bias_strides = compute_strides(bias_sizes);
207216

208217
std::map<char, std::size_t> pos;
209218
for (std::size_t out_idx = 0; out_idx < out.size(); ++out_idx) {
210219
decode_index(out_idx, rhs_, rhs_strides, pos);
211-
out[out_idx] += bias_vals[encode_offset(bias_axes_, bias_strides, pos)];
220+
out[out_idx] += bias_vals[encode_offset(bias_chars, bias_strides, pos)];
212221
}
213222
}
214223

include/fdeep/layers/group_normalization_layer.hpp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,13 +49,15 @@ namespace internal {
4949
"GroupNormalization is currently only supported on the last (channel) axis.");
5050

5151
const std::size_t channels = in_shape.depth_;
52+
assertion(groups_ > 0, "GroupNormalization groups must be > 0.");
5253
assertion(channels % groups_ == 0,
5354
"Number of channels must be divisible by number of groups.");
5455
const std::size_t channels_per_group = channels / groups_;
5556

5657
const std::size_t spatial_volume = in_shape.size_dim_5_
5758
* in_shape.size_dim_4_ * in_shape.height_ * in_shape.width_;
5859
const std::size_t group_volume = spatial_volume * channels_per_group;
60+
assertion(group_volume > 0, "GroupNormalization input has zero volume.");
5961

6062
const auto& src = *input.as_vector();
6163
float_vec means(groups_, 0);

include/fdeep/layers/integer_lookup_layer.hpp

Lines changed: 18 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -28,23 +28,31 @@ namespace internal {
2828
, mask_token_(mask_token)
2929
, num_oov_indices_(num_oov_indices)
3030
, vocab_offset_((has_mask_token ? 1 : 0) + num_oov_indices)
31-
, vocab_index_()
31+
, vocab_index_(build_vocab_index(vocabulary, num_oov_indices))
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.");
38-
for (std::size_t i = 0; i < vocabulary.size(); ++i)
39-
vocab_index_[vocabulary[i]] = i;
4033
}
4134

4235
protected:
4336
const bool has_mask_token_;
4437
const std::int64_t mask_token_;
4538
const std::size_t num_oov_indices_;
4639
const std::size_t vocab_offset_;
47-
std::unordered_map<std::int64_t, std::size_t> vocab_index_;
40+
const std::unordered_map<std::int64_t, std::size_t> vocab_index_;
41+
42+
// num_oov_indices > 1 needs Keras's FarmHash, which fdeep does not
43+
// currently replicate. num_oov_indices == 0 ("strict" mode) raises on
44+
// OOV, also unsupported. Require exactly one OOV bucket.
45+
static std::unordered_map<std::int64_t, std::size_t> build_vocab_index(
46+
const std::vector<std::int64_t>& vocabulary, std::size_t num_oov_indices)
47+
{
48+
assertion(num_oov_indices == 1,
49+
"IntegerLookup requires num_oov_indices == 1.");
50+
std::unordered_map<std::int64_t, std::size_t> idx;
51+
idx.reserve(vocabulary.size());
52+
for (std::size_t i = 0; i < vocabulary.size(); ++i)
53+
idx[vocabulary[i]] = i;
54+
return idx;
55+
}
4856

4957
tensors apply_impl(const tensors& inputs) const override
5058
{
@@ -61,8 +69,7 @@ namespace internal {
6169
if (it != vocab_index_.end()) {
6270
out[i] = static_cast<float_type>(vocab_offset_ + it->second);
6371
} else {
64-
// num_oov_indices_ <= 1 (asserted in ctor): OOV maps to 0,
65-
// shifted by 1 if a mask token occupies index 0.
72+
// OOV maps to 0, shifted by 1 if a mask token occupies index 0.
6673
out[i] = static_cast<float_type>(has_mask_token_ ? 1 : 0);
6774
}
6875
}

include/fdeep/recurrent_ops.hpp

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,7 @@ namespace internal {
125125
const std::string& activation,
126126
const std::string& recurrent_activation)
127127
{
128+
assertion(n_units > 0, "LSTM units must be > 0.");
128129
const MappedRowMajorMatrixXf W = eigen_row_major_mat_from_shared_values(
129130
weights.size() / (n_units * 4), n_units * 4,
130131
weights.data());
@@ -205,6 +206,7 @@ namespace internal {
205206
const std::string& activation,
206207
const std::string& recurrent_activation)
207208
{
209+
assertion(n_units > 0, "GRU units must be > 0.");
208210
const std::size_t n_timesteps = input.shape().width_;
209211
const std::size_t n_features = input.shape().depth_;
210212

@@ -300,6 +302,7 @@ namespace internal {
300302
const float_vec& bias,
301303
const std::string& activation)
302304
{
305+
assertion(n_units > 0, "SimpleRNN units must be > 0.");
303306
const std::size_t n_timesteps = input.shape().width_;
304307
const std::size_t n_features = input.shape().depth_;
305308

keras_export/generate_test_models.py

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,8 @@
4242
from keras.layers import RNN, LSTMCell, GRUCell, SimpleRNNCell, StackedRNNCells
4343
from keras.layers import Discretization, IntegerLookup, Masking
4444
from keras.layers import RandomBrightness, RandomFlip, RandomCrop
45+
from keras.layers import RandomContrast, RandomRotation, RandomTranslation, RandomZoom
46+
from keras.layers import RandomHue, RandomSaturation, RandomSharpness
4547
from keras.models import Model, load_model, Sequential
4648

4749
__author__ = "Tobias Hermann"
@@ -546,6 +548,10 @@ def get_test_model_exhaustive() -> Model:
546548
num_key_value_heads=2, use_gate=True)(inputs[49], inputs[49], inputs[49]))
547549
outputs.append(GroupQueryAttention(head_dim=4, num_query_heads=4,
548550
num_key_value_heads=4)(inputs[49], inputs[49], inputs[49]))
551+
# num_query_heads == num_key_value_heads with distinct K/V seq lengths
552+
# exercises the MHA-equivalence path with cross-attention shapes.
553+
outputs.append(GroupQueryAttention(head_dim=4, num_query_heads=4,
554+
num_key_value_heads=4)(inputs[49], inputs[50], inputs[51]))
549555
outputs.append(GroupQueryAttention(head_dim=3, num_query_heads=4,
550556
num_key_value_heads=2)(inputs[49], inputs[50], inputs[51]))
551557

@@ -587,10 +593,21 @@ def get_test_model_exhaustive() -> Model:
587593
# Discretization on a float input.
588594
outputs.append(Discretization(bin_boundaries=[-0.5, 0.0, 0.5, 1.0])(inputs[49]))
589595

596+
# IntegerLookup: scaled to int via Discretization so the input is float.
597+
outputs.append(IntegerLookup(vocabulary=[0, 1, 2, 3])(
598+
Discretization(bin_boundaries=[-1.0, 0.0, 1.0])(inputs[49])))
599+
590600
# Training-only Random* augmentation layers (passed through at inference).
591601
outputs.append(RandomBrightness(factor=0.1)(inputs[22]))
592602
outputs.append(RandomFlip(mode='horizontal')(inputs[22]))
593603
outputs.append(RandomCrop(height=26, width=28)(inputs[22]))
604+
outputs.append(RandomContrast(factor=0.1)(inputs[22]))
605+
outputs.append(RandomRotation(factor=0.1)(inputs[22]))
606+
outputs.append(RandomTranslation(0.1, 0.1)(inputs[22]))
607+
outputs.append(RandomZoom(0.1)(inputs[22]))
608+
outputs.append(RandomHue(factor=0.1, value_range=(0.0, 1.0))(inputs[22]))
609+
outputs.append(RandomSaturation(factor=0.1, value_range=(0.0, 1.0))(inputs[22]))
610+
outputs.append(RandomSharpness(factor=0.1, value_range=(0.0, 1.0))(inputs[22]))
594611

595612
shared_conv = Conv2D(1, (1, 1),
596613
padding='valid', name='shared_conv', activation='relu')
@@ -901,6 +918,9 @@ def get_test_model_recurrent() -> Model:
901918
rnn_seq = SimpleRNN(6, return_sequences=True)(inputs)
902919
rnn_last = SimpleRNN(5, activation='tanh')(rnn_seq)
903920
rnn_relu_no_bias = SimpleRNN(4, activation='relu', use_bias=False)(inputs)
921+
rnn_state_out, rnn_state_h = SimpleRNN(3, return_state=True)(inputs)
922+
923+
gru_state_out, gru_state_h = GRU(4, return_state=True)(inputs)
904924

905925
bidi_lstm_seq = Bidirectional(LSTM(4, return_sequences=True))(inputs)
906926
bidi_lstm_last_concat = Bidirectional(LSTM(3))(bidi_lstm_seq)
@@ -941,8 +961,8 @@ def get_test_model_recurrent() -> Model:
941961
outputs = [
942962
lstm_last, lstm_no_bias, lstm_relu, lstm_no_unit_forget,
943963
lstm_state_out, lstm_state_h, lstm_state_c,
944-
gru_last, gru_no_bias,
945-
rnn_last, rnn_relu_no_bias,
964+
gru_last, gru_no_bias, gru_state_out, gru_state_h,
965+
rnn_last, rnn_relu_no_bias, rnn_state_out, rnn_state_h,
946966
bidi_lstm_last_concat,
947967
bidi_lstm_sum,
948968
bidi_gru_mul,

0 commit comments

Comments
 (0)