Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Keras 3.3.3 upgrade #165

Conversation

Chase-Grajeda
Copy link

  • Included Keras 3.3.3 upgrades for all network classes and instances of tf.model, tf.layer, and tf.optimizer
  • Added @tf.function decorator to coupling_networks._calculate_spline() to allow execution in non-eager environments
  • Changed instances of tf.Tensor in docstring to Tensor w.r.t. the experimental Tensor type

The included changes have worked successfully in all of the example / tutorial notebooks, however I'm sure there are a few introduced bugs laying around when TensorFlow is not the chosen backend.

Current environment dependencies:

  • keras 3.3.3
  • tensorflow 2.16.1
  • tensorflow-intel 2.16.1
  • tensorflow-probability 0.24.0

Rolled over changes for Keras 3.3 for all tf.Model classes
Copy link
Contributor

@LarsKue LarsKue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice changes overall, thank you! See comments for some minor changes before merging.

@@ -436,7 +439,7 @@ def _semantic_spline_parameters(self, parameters):
"""

shape = tf.shape(parameters)
rank = len(shape)
rank = parameters.shape.rank
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tensorflow-specific, len(shape) or parameters.ndim should be fine. Safest would probably be keras.ops.ndim

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed parameters.shape.rank to use keras.ops.ndim in coupling_networks.py (see 47fbbbd)

# LearningRateSchedule instances need number of iterations
current_lr = optimizer.lr(optimizer.iterations).numpy()
elif hasattr(optimizer.lr, "numpy"):
current_lr = optimizer.learning_rate(optimizer.iterations).numpy()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if this works under jax/numpy backend - can you test this? As an alternative, we could use keras.ops.convert_to_numpy(). We can then drop the hasattr check

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed learning_rate().numpy() to keras.ops.convert_to_numpy() in helper_functions.py (see 47fbbbd)

target_shape = tf.shape(targets)
condition_shape = tf.shape(condition)
target_shape = keras.backend.shape(targets)
condition_shape = keras.backend.shape(condition)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as discussed, please use keras.ops

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed requested keras.backend instances to keras.ops in inference_networks.py. (see 47fbbbd)
Will change remaining keras.backend to keras.ops calls with next update

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remaining keras.backend calls switched to respective keras.ops functions (see b9d5f16)

@Chase-Grajeda Chase-Grajeda self-assigned this May 23, 2024
Changed parameters.shape.rank to use keras.ops.ndim in coupling_networks.py,
Changed learning_rate.numpy() to keras.ops.convert_to_numpy() in helper_functions.py,
Changed requested keras.backend instances to keras.ops in inference_networks.py.
Changed all calls of keras.backend to the respective keras.ops function
@LarsKue LarsKue requested a review from stefanradev93 June 3, 2024 14:45
@LarsKue
Copy link
Contributor

LarsKue commented Jun 24, 2024

We cannot merge this anymore due to the many changes on the branch, but we'll use this as a reference nonetheless.

@LarsKue LarsKue closed this Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants