-
Notifications
You must be signed in to change notification settings - Fork 330
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
Port CenterPillar to Keras Core #2003
Conversation
/gcbrun |
/gcbrun |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ianstenbit I thought this model wasn't possible to port due to custom tf ops and I obviously see a lot of TF code remaining. Is the idea that this is a TensorFlow-only model that works with Keras Core? If so, is this something we can flag for users?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file still has a LOT of tensorflow
code, is that intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! This lives outside of the model and runs in tf.data
def point_to_voxel_coord( | ||
point_xyz: tf.Tensor, voxel_size: Sequence[float], dtype=tf.int32 | ||
) -> tf.Tensor: | ||
def point_to_voxel_coord(point_xyz, voxel_size, dtype=tf.int32): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function also has a lot of TF
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Same here -- this is for tf.data
)
This does work with other backends, but requires data encoding in a tf.data pipeline, so that code is all still written in TF. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Feel free to merge whenever, just left some random low level comments for things that popped out.
/gcbrun |
/gcbrun |
/gcbrun |
* Port CenterPillar to Keras Core * Switch back to backbone API * Restructure center_pillar.py * Undo some mistaken changes * More fixes -- tests currently failing * Test passing across backends * Remove local segment_max op * Finish port + tests passing * Fix tests + format * Remove numpy behavior * Moar test fixes * Use smaller backbone * Review comments * Add 3D OD models to multi-backend CI
TODOs (all done now -- sending for review)
_core
versions of voxel_utilssegment_max
keras_core.backend.name_scope
center_pillar.py