keras icon indicating copy to clipboard operation
keras copied to clipboard

mlx - implement segment_sum and segment_max

Open lkarthee opened this issue 1 year ago • 5 comments

lkarthee avatar May 01 '24 08:05 lkarthee

Codecov Report

Attention: Patch coverage is 0% with 22 lines in your changes are missing coverage. Please review.

:exclamation: No coverage uploaded for pull request base (mlx@4c90dfb). Click here to learn what that means.

Files Patch % Lines
keras/src/backend/mlx/math.py 0.00% 22 Missing :warning:
Additional details and impacted files
@@          Coverage Diff           @@
##             mlx   #19652   +/-   ##
======================================
  Coverage       ?   68.43%           
======================================
  Files          ?      506           
  Lines          ?    45959           
  Branches       ?     8496           
======================================
  Hits           ?    31451           
  Misses         ?    12857           
  Partials       ?     1651           
Flag Coverage Δ
keras 68.35% <0.00%> (?)
keras-jax 58.82% <0.00%> (?)
keras-numpy 53.16% <0.00%> (?)
keras-tensorflow 59.97% <0.00%> (?)

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar May 01 '24 09:05 codecov-commenter

Hi @lkarthee,

I have noticed the same issue: there are missing and needed functions in MLX.

What is the best strategy here? Should we use NumPy temporarily and add a TODO comment?

I have decided to start with core files and functions. https://github.com/keras-team/keras/pull/19619

I suggest creating a roadmap where we start with fundamental functions and then move up.

best

Faisal-Alsrheed avatar May 01 '24 09:05 Faisal-Alsrheed

@Faisal-Alsrheed My thoughts regarding missing functions which can't be added due to design decisions/limitations of mlx:

  • we have to check if there is a workaround.
  • if there is no workaround, may be we have to fallback to using numpy or jax adding a TODO?

Regarding fixing core first, I have been doing that in my PRs. I tried to implement Pooling and CNN related funcs, realised many tests are failing and started fixing test cases.

@fchollet any thoughts on what to use as fallback - numpy or jax.

lkarthee avatar May 01 '24 12:05 lkarthee

Hi @fchollet Any update on this PR? Please. Thank you!

gbaned avatar Jul 12 '24 03:07 gbaned

@fchollet any thoughts on what to use as fallback - numpy or jax.

Sorry, just checking this now. I would say that JAX is preferable as fallback since numpy would not be sufficiently performant.

fchollet avatar Jul 13 '24 17:07 fchollet

Hi @lkarthee Any update on this PR? Please. Thank you!

gbaned avatar Sep 09 '24 08:09 gbaned

This PR is stale because it has been open for 14 days with no activity. It will be closed if no further activity occurs. Thank you.

github-actions[bot] avatar Sep 24 '24 02:09 github-actions[bot]

This PR was closed because it has been inactive for 28 days. Please reopen if you'd like to work on this further.

github-actions[bot] avatar Oct 08 '24 02:10 github-actions[bot]