keras icon indicating copy to clipboard operation
keras copied to clipboard

Updated Complex dtype input support for layers

Open mehtamansi29 opened this issue 1 year ago • 6 comments

Updated complex dtype for ALLOWED_DTYPES=complex64,complex128 in the documentation. Please have a look at the changes and do the needful. Thank you. Fixes #19860

mehtamansi29 avatar Jun 18 '24 06:06 mehtamansi29

Thanks for the PR! We'd need to add some test with a custom layer with complex inputs to verify that it works (and make sure we won't break it in the future). Also, current tests seem to be failing.

fchollet avatar Jun 18 '24 15:06 fchollet

Hi, Is there any progress on this feature? Having layers with complex inputs is a crucial requirement for many areas of application. Any information on this issue would be appreciated. Thanks!

jhoydis avatar Jul 10 '24 14:07 jhoydis

Hi @jhoydis -

I am working on adding some test with a custom layer with complex inputs. Will update with successful test once its done. Thanks..!!

mehtamansi29 avatar Jul 23 '24 06:07 mehtamansi29

Hi, thanks for looking into this - just some comment: I tried applying the proposed fix (add complex64/128 in ALLOWED_DTYPES) to activate complex type support for layers, but noticed this doesn't fix if fully: using slicing operation (I used layers.__getitem__ through [..] syntax, but believe this maps to any other tf.slice or tf.strided_slice) with complex64 type input results in float32 type output instead of expected complex64. Kind regards!

arnefib avatar Aug 08 '24 05:08 arnefib

So what shall we do with this PR?

fchollet avatar Aug 12 '24 18:08 fchollet

So what shall we do with this PR?

Hi @fchollet - Sorry for the delay. I am working on adding test for complex input. I'll update soon(maybe today or tomorrow), once newly added test case will get passed. Thanks..!!

mehtamansi29 avatar Aug 19 '24 07:08 mehtamansi29

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 73.39%. Comparing base (befa049) to head (c7f98d9). Report is 131 commits behind head on master.

:exclamation: There is a different number of reports uploaded between BASE (befa049) and HEAD (c7f98d9). Click for more details.

HEAD has 2 uploads less than BASE
Flag BASE (befa049) HEAD (c7f98d9)
keras 4 3
keras-torch 1 0
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #19872      +/-   ##
==========================================
- Coverage   79.35%   73.39%   -5.96%     
==========================================
  Files         501      509       +8     
  Lines       47311    48277     +966     
  Branches     8689     8883     +194     
==========================================
- Hits        37542    35432    -2110     
- Misses       8014    11063    +3049     
- Partials     1755     1782      +27     
Flag Coverage Δ
keras 73.32% <100.00%> (-5.89%) :arrow_down:
keras-jax 62.28% <100.00%> (-0.19%) :arrow_down:
keras-numpy 57.43% <100.00%> (-0.15%) :arrow_down:
keras-tensorflow 63.63% <100.00%> (-0.23%) :arrow_down:
keras-torch ?

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 Aug 29 '24 01:08 codecov-commenter

Are you doing ok with the test fixes across the different backends? I see that TF is passing at least, that's good!

fchollet avatar Aug 29 '24 17:08 fchollet

Are you doing ok with the test fixes across the different backends? I see that TF is passing at least, that's good!

Hi @fchollet - For Torch backend I am not doing test fixes for complex dtype as torch doesn't support it. And for JAX backend running the failed test in my local same test went passed. But when commit it, that is failing. But as TF passing is good and custom layer with complex input case is working fine then can I leave it different backend(JAX and torch) as it is ?

mehtamansi29 avatar Aug 30 '24 00:08 mehtamansi29

For Torch backend I am not doing test fixes for complex dtype as torch doesn't support it.

Then you should explicitly exclude torch from these unit tests via a pytest skip decorator.

And for JAX backend running the failed test in my local same test went passed. But when commit it, that is failing.

What is the issue here?

fchollet avatar Sep 03 '24 17:09 fchollet

Thanks for the update.

This branch has conflicts that must be resolved

Can you please solve the conflicts?

fchollet avatar Sep 19 '24 16:09 fchollet

Thanks for the update.

This branch has conflicts that must be resolved

Can you please solve the conflicts?

Sure @fchollet - I'll resolve it and update the commit by tomorrow. Every test case got passes except torch(even after adding pytest skip decorator in keras/src/backend/common/dtypes_test.py file).

mehtamansi29 avatar Sep 19 '24 18:09 mehtamansi29

Thanks, I'll fix the remainder forward.

Thanks @fchollet..!!

mehtamansi29 avatar Sep 22 '24 16:09 mehtamansi29

Please see #20278 for a related issue that seems to be introduced with this merge.

jhoydis avatar Sep 23 '24 11:09 jhoydis