scan added #18515

vulkomilev · 2023-09-27T17:33:24Z

scan functionality added

fchollet

Thank you for the PR! 👍

fchollet · 2023-09-27T18:36:00Z

keras/backend/tensorflow/core.py

+         length=None,
+         reverse=False,
+         unroll=1):
+    return tf.scan(


Please format the code via sh shell/format.sh

fchollet · 2023-09-27T18:36:42Z

keras/ops/core.py

@@ -204,6 +204,16 @@ def slice_update(inputs, start_indices, updates):
    return backend.core.slice_update(inputs, start_indices, updates)


+@keras_export("keras.ops.scan")


There should be a correspond symbolic op class.

Also, please add a docstring for this function.

done.I am not sure about the corresponding symbolic class

fchollet · 2023-09-27T18:37:25Z

keras/ops/core_test.py

@@ -215,6 +215,17 @@ def test_slice_update(self):
        outputs = core.slice_update(inputs, start_indices, updates)
        self.assertAllClose(outputs[1:3, 1:3, 2:4, 2:4], np.zeros([2, 2, 2, 2]))

+
+    def test_scan(self):


Let's use jax.scan as the reference and let's test consistency with the reference across multiple inputs and dtypes. Let's also test static shape inference.

I have added two tests one with int and one with float. The float is not passing due to rounding errors. Should I round it to remove such errors . Also I am not sure about what do you mean by static shape inference sorry.

Also I am not sure about what do you mean by static shape inference sorry.

This means testing the output shape and dtypes obtained when calling the op on a KerasTensor. There is a test case for this in this file.

The test class is called CoreOpsStaticShapeTest and is right above this one. You can take a look at some of the test cases for the class to get an idea of how static shape inference testing works.

fchollet

Thanks for the updates!

fchollet · 2023-10-10T22:41:27Z

keras/backend/tensorflow/core.py

+    if reverse:
+        np.flip(xs)
+
+    init = (init, np.array(0, dtype=init.dtype))


You can use tf.zeros_like instead of np.array

fchollet · 2023-10-10T22:41:49Z

keras/backend/tensorflow/core.py

+
+    carry, ys = tf.scan(f, xs, initializer=init)
+
+    ys = ys.numpy()


Don't convert to numpy, keep TF tensors throughout.

fchollet · 2023-10-10T22:42:12Z

keras/backend/tensorflow/core.py

+    if xs is None:
+        xs = [None] * length
+    if reverse:
+        np.flip(xs)


Use a TF op for this

fchollet · 2023-10-10T22:42:29Z

keras/backend/torch/core.py

+        ys.append(y)
+    if reverse:
+        ys.reverse()
+    return carry, np.stack(ys)


Use torch ops, not np

fchollet · 2023-10-10T22:43:37Z

keras/ops/core_test.py

@@ -215,6 +215,17 @@ def test_slice_update(self):
        outputs = core.slice_update(inputs, start_indices, updates)
        self.assertAllClose(outputs[1:3, 1:3, 2:4, 2:4], np.zeros([2, 2, 2, 2]))

+
+    def test_scan(self):


Also I am not sure about what do you mean by static shape inference sorry.

This means testing the output shape and dtypes obtained when calling the op on a KerasTensor. There is a test case for this in this file.

codecov-commenter · 2023-10-10T22:55:36Z

Codecov Report

Attention: 51 lines in your changes are missing coverage. Please review.

Comparison is base (10252a9) 63.86% compared to head (71d3173) 55.65%.
Report is 84 commits behind head on master.

Files	Patch %	Lines
keras/backend/tensorflow/core.py	5.26%	18 Missing ⚠️
keras/backend/torch/core.py	0.00%	17 Missing ⚠️
keras/backend/numpy/core.py	52.17%	5 Missing and 6 partials ⚠️
keras/ops/core.py	60.00%	3 Missing and 1 partial ⚠️
keras/backend/jax/core.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #18515      +/-   ##
==========================================
- Coverage   63.86%   55.65%   -8.21%     
==========================================
  Files         336      339       +3     
  Lines       34848    35689     +841     
  Branches     6855     7032     +177     
==========================================
- Hits        22255    19864    -2391     
- Misses      11121    14335    +3214     
- Partials     1472     1490      +18

Flag	Coverage Δ
keras	`55.65% <28.16%> (-8.21%)`	⬇️
keras-numpy	`55.65% <28.16%> (?)`
keras-torch	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

vulkomilev · 2023-10-17T17:52:15Z

@fchollet I have made some changes

fchollet

Thanks for the updates!

fchollet · 2023-10-17T21:27:14Z

keras/backend/numpy/core.py

@@ -204,6 +204,22 @@ def while_loop(
    return loop_vars


+def scan(f, init, xs, length=None, reverse=False, unroll=1):


The unroll arg is unused

fchollet · 2023-10-17T21:27:20Z

keras/backend/tensorflow/core.py

@@ -194,6 +194,22 @@ def slice_update(inputs, start_indices, updates):
    return dynamic_update_slice(inputs, updates, start_indices)


+def scan(f, init, xs, length=None, reverse=False, unroll=1):


fchollet · 2023-10-17T21:27:25Z

keras/backend/torch/core.py

@@ -335,6 +335,22 @@ def slice_update(inputs, start_indices, updates):
    return outputs


+def scan(f, init, xs, length=None, reverse=False, unroll=1):


fchollet · 2023-10-18T08:24:31Z

shell/format.sh

@@ -1,5 +1,5 @@
 #!/bin/bash
-set -Eeuo pipefail
+set -Eeu pipefail


You can revert this

fchollet · 2023-10-18T08:25:02Z

keras/ops/core_test.py

+            ys_op[i] = round(ys_op[0],2)
+            ys_jax[i] = round(ys_jax[0], 2)
+
+        self.assertListEqual(ys_op, ys_jax)


Use assertAllClose for numerical checks.

Please test more input argument configurations. You could use a parameterize test for that.

New test added. I couldn't use parameterize because I can't pass arguments.I used instead for cycle.

vulkomilev · 2023-10-23T18:06:51Z

New commit

fchollet · 2023-11-02T23:30:32Z

Thanks for the update. Can you get the tests to pass?

vulkomilev · 2023-11-09T16:59:52Z

I think that test are passing now @fchollet

fchollet · 2023-11-11T00:54:34Z

keras/ops/core.py

+        return backend.core.scan(f, init, xs, length, reverse, unroll)
+
+    def compute_output_spec(self, f, init, xs, length, reverse, unroll):
+        return KerasTensor(xs.shape, dtype=xs.dtype)


This doesn't look correct -- it should match the shape of the output tensors. To start with, there are 2 tensors in this case.

okay I have added 2 tensors to the return statement but I am not sure if they are correct

keras/ops/core.py

fchollet · 2023-11-11T00:55:19Z

keras/ops/core.py

@@ -204,6 +212,46 @@ def slice_update(inputs, start_indices, updates):
    return backend.core.slice_update(inputs, start_indices, updates)


+@keras_export("keras.ops.scan")
+def scan(f, init, xs, length=None, reverse=False):


You can just add unroll everywhere for consistency, even if it's not used by some backends.

fchollet · 2023-11-11T00:56:24Z

keras/ops/core_test.py

@@ -215,6 +215,17 @@ def test_slice_update(self):
        outputs = core.slice_update(inputs, start_indices, updates)
        self.assertAllClose(outputs[1:3, 1:3, 2:4, 2:4], np.zeros([2, 2, 2, 2]))

+
+    def test_scan(self):


The test class is called CoreOpsStaticShapeTest and is right above this one. You can take a look at some of the test cases for the class to get an idea of how static shape inference testing works.

gbaned · 2023-11-15T03:47:33Z

Hi @vulkomilev Can you please check @fchollet's comments and resolve conflicts?. Thank you!

vulkomilev · 2023-11-15T13:55:57Z

yes I will do it today

vulkomilev · 2023-11-15T19:24:36Z

@fchollet I think that I have scan test in CoreOpsStaticShapeTest

gbaned · 2023-12-01T08:05:04Z

Hi @vulkomilev Can you please resolve conflicts? Thank you!

vulkomilev · 2023-12-02T16:37:27Z

@gbaned done

fchollet

Congrats on getting the tests to pass! This would still need a Scan class and tests for static shape inference.

fchollet · 2023-12-11T18:54:04Z

keras/backend/tensorflow/core.py

+
+    carry, ys = tf.scan(f, xs, initializer=init)
+
+    # if carry[0].dtype is tf.float64:


If the code is no longer useful, please remove it

fchollet · 2023-12-11T18:54:59Z

keras/ops/core.py

+        A scanned array and a carry element.
+    """
+    if any_symbolic_tensors((init, xs)):
+        return SliceUpdate().symbolic_call(f, init, xs, length, reverse, unroll)


So this should be Scan. Then there should be a class Scan object definition, similar to the other classes in this file

gbaned · 2023-12-29T09:11:06Z

Hi @vulkomilev Can you please check @fchollet's comments and keep us posted ? Thank you!

vulkomilev · 2024-01-08T21:40:52Z

@gbaned done

scan added

5594501

google-ml-butler bot added the size:M label Sep 27, 2023

google-ml-butler bot assigned gbaned Sep 27, 2023

fchollet reviewed Sep 27, 2023

View reviewed changes

work in progress

4051472

gbaned requested a review from fchollet October 6, 2023 05:57

google-ml-butler bot added the awaiting review label Oct 6, 2023

updated documentation formated the code added a tests

b83a8d8

fchollet reviewed Oct 10, 2023

View reviewed changes

some fixes

aed1f55

fchollet reviewed Oct 18, 2023

View reviewed changes

some fixes

bcfc456

sachinprasadhs added the stat:awaiting keras-eng Awaiting response from Keras engineer label Oct 25, 2023

vulkomilev added 2 commits November 8, 2023 21:38

cores fixed

e756750

some fixes with regards to CoreOpsStaticShapeTest

f40911a

gbaned removed the awaiting review label Nov 10, 2023

fchollet reviewed Nov 11, 2023

View reviewed changes

gbaned added stat:awaiting response from contributor and removed stat:awaiting keras-eng Awaiting response from Keras engineer labels Nov 15, 2023

google-ml-butler bot removed the stat:awaiting response from contributor label Nov 15, 2023

fixes according to the comments

d5b1e8f

sachinprasadhs added the stat:awaiting keras-eng Awaiting response from Keras engineer label Nov 15, 2023

gbaned added stat:awaiting response from contributor and removed stat:awaiting keras-eng Awaiting response from Keras engineer labels Dec 1, 2023

Merge branch 'master' into master

2fae37e

google-ml-butler bot removed the stat:awaiting response from contributor label Dec 2, 2023

gbaned requested a review from fchollet December 4, 2023 04:22

google-ml-butler bot added the awaiting review label Dec 4, 2023

vulkomilev added 2 commits December 5, 2023 19:08

format fixed

ef216c4

fixed default argument

611e974

fchollet reviewed Dec 11, 2023

View reviewed changes

gbaned added stat:awaiting response from contributor and removed awaiting review labels Dec 29, 2023

fixes according to comments

71d3173

google-ml-butler bot removed the stat:awaiting response from contributor label Jan 8, 2024

gbaned requested a review from fchollet January 9, 2024 00:42

google-ml-butler bot added the awaiting review label Jan 9, 2024

sachinprasadhs added the stat:awaiting keras-eng Awaiting response from Keras engineer label Jan 17, 2024

vulkomilev closed this by deleting the head repository Apr 18, 2024

google-ml-butler bot removed the awaiting review label Apr 18, 2024

		@@ -204,6 +204,16 @@ def slice_update(inputs, start_indices, updates):
		return backend.core.slice_update(inputs, start_indices, updates)


		@keras_export("keras.ops.scan")

		@@ -204,6 +204,22 @@ def while_loop(
		return loop_vars


		def scan(f, init, xs, length=None, reverse=False, unroll=1):

		@@ -194,6 +194,22 @@ def slice_update(inputs, start_indices, updates):
		return dynamic_update_slice(inputs, updates, start_indices)


		def scan(f, init, xs, length=None, reverse=False, unroll=1):

		@@ -335,6 +335,22 @@ def slice_update(inputs, start_indices, updates):
		return outputs


		def scan(f, init, xs, length=None, reverse=False, unroll=1):


		carry, ys = tf.scan(f, xs, initializer=init)

		# if carry[0].dtype is tf.float64:

scan added #18515

scan added #18515

Conversation

vulkomilev commented Sep 27, 2023

fchollet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fchollet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 10, 2023 • edited Loading

Codecov Report

vulkomilev commented Oct 17, 2023

fchollet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vulkomilev commented Oct 23, 2023

fchollet commented Nov 2, 2023

vulkomilev commented Nov 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbaned commented Nov 15, 2023

vulkomilev commented Nov 15, 2023

vulkomilev commented Nov 15, 2023

gbaned commented Dec 1, 2023

vulkomilev commented Dec 2, 2023

fchollet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbaned commented Dec 29, 2023

vulkomilev commented Jan 8, 2024

codecov-commenter commented Oct 10, 2023 •

edited

Loading