feat: support integer dtypes in ListMax - #74
Conversation
georyetti
left a comment
There was a problem hiding this comment.
Now we have int support (especially int8) the neg_inf pattern is not robust anymore. Hoping we can refactor quickly to a mask pattern
There was a problem hiding this comment.
neg_inf for int8 is actually -128. Which could reasonably be a valid value.
Wondering if we can refactor this more generally and just keep the boolean mask and reuse it later.
There was a problem hiding this comment.
If we're doing this kind of refactor to use the mask rather than setting a neg_inf value, shouldn't that be something we do more generally as a consistent pattern across all transformers @georyetti?
There was a problem hiding this comment.
Maybe for now we could stick to higher precision int types for this one
The layer decided whether a filter had emptied a segment by testing the reduction result against dtype.min, the neutral element it substitutes for filtered entries. That reads as a sentinel, but on the narrow integer dtypes it is ordinary data: a segment whose genuine maximum is -128 on int8 was indistinguishable from one the filter had emptied, and the real value was overwritten with nanFillValue. Track which entries survive the filter in a mask and reduce that alongside the values, so emptiness is answered directly instead of inferred from the result. This mirrors the approach taken in ListSum. Comparing an integer tensor against the float min_filter_value also raised a TypeError, so integer thresholds are now narrowed explicitly. Rounding up leaves >= unchanged on integers, and a threshold beyond the bounds of the dtype is resolved without a cast, which would otherwise wrap around and silently invert the comparison. The shared min_filter_mask helper holds this logic so ListSum can adopt it. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@dummy1cx Can you condense this to a summary please? Ideally without the use of AI |
|
Hey George! I have swapped the neg_inf check with a mask in the layer, so it keeps track of which values passed the filter and checks that at the end. Now a real -128 value comes back as the max instead of being replaced with nan_fill_value. But while testing I got some bugs in the code, as filtering an int column was throwing a TypeError because min_filter_value is a float, so the threshold gets converted now. My first go at that wrapped around when the threshold was outside the dtype range (200 becomes -56 on int8), so those are handled separately without a cast. I have run the test files covering the Spark/TF match, and the full suite passed locally. |
Description
Widens
ListMaxto accept integer inputs, and fixes a latent bug in themin_filter_valuecode path that this exposed.1. Integer dtypes
ListMaxTransformer/ListMaxLayerpreviously accepted only float and stringdtypes. Taking a listwise max over an integer column is a natural operation, but
it required an upstream cast to double purely to satisfy the dtype check. This
adds
ByteType/ShortType/IntegerType/LongTypeon the Spark side andint8/int16/int32/int64on the Keras side.The motivating use case is count features (e.g. number of properties, number of
searches) stored as
bigint, where we want the max within a query/segment whilekeeping the column integral.
2. Bug fix in the
min_filter_valuepathListMaxLayermasks filtered-out values withdtype.minand then substitutesnan_fill_valuefor any segment that ends up empty:nan_fill_valueis a Python float, andtf.constant(0.0, dtype=tf.int64)raisesTypeError: Cannot convert 0.0 to EagerTensor of dtype int64. So simply wideningthe dtypes would have made the layer crash whenever
min_filter_valuewas set onan integer column. This was unreachable before this PR, since integer inputs were
rejected by the dtype check.
The value is now narrowed through numpy before constructing the tensor:
Note that
tf.cast(self.nan_fill_value, listwise_max.dtype)is the more obviousfix, but it silently loses precision on
float64: a Python float is converted toa
float32tensor first and then widened, so a fill value of123.456comes backas
123.45600128173828.I compared the numpy narrowing against the previous
tf.constantbehaviour over~4000 values per dtype, comparing raw bytes rather than numeric equality.
float64is identical throughout. The only differences anywhere are:-0.0, which the old path normalised to+0.0and the new path preserves.The two compare equal, so there is no numeric impact.
bfloat16(e.g.1e-38), which the old path flushed tozero and the new path represents.
Both are cases where the new behaviour is at least as faithful as the old, and
neither is reachable with a realistic fill value, so existing users are
unaffected.
Testing
Every integer dtype added here is covered on both backends:
int64with segmentation, and for each ofint8,int16,int32andint64with segmentation plus amin_filter_valuethatempties a segment entirely. That last scenario is the one that exercises the
nan_fill_valuefix, so it is covered for every width rather than just one.tinyint,smallint,intandbigint, confirming the two backends agree on integersincluding when the fill value is applied.
float64andfloat32with values that are not exactly representable infloat32, plusint64andint32. This fails against thetf.castapproachdescribed above and guards the float precision behaviour.
Checklists
ListMaxis an existing transformer, so most items in the new-transformerchecklists do not apply. The two that are relevant:
compatible_dtypesproperty has been implemented to specify theinput/output data types that my transformer/estimator supports.
tests between the Spark and Keras implementations.
No README entry, as this is not a new transformer.
Note for reviewers
Was the original float-and-string restriction deliberate? I could not find a
reason for it in the code or history, and the underlying ops
(
tf.math.unsorted_segment_max,F.max) both handle integers, but happy toadjust if there was intent behind it.