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

fix(layers): Fix compute_dtype handling for complex-valued inputs in Dense layer #20823

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

harshaljanjani
Copy link
Contributor

Fixed incompatibility between compute_dtype and complex-valued inputs in the Dense layer (issue #20278). The layer previously did not properly handle dtype promotion when mixing compute_dtype with complex inputs.

Key changes:

  • Add proper dtype promotion in Dense.call() using backend.result_type()
  • Cast inputs, kernel and bias weights to the promoted dtype.
  • Add complex number initialization support via initializer_for_complex wrapper
  • Add comprehensive test suite for complex number handling

This ensures the Dense layer correctly handles:

  • Complex-valued inputs with different compute_dtype settings
  • Mixed real and complex computations
  • Complex weight initialization
  • Complex number training

Tests added verify compatibility across different backends (TensorFlow, JAX, PyTorch) and complex number use cases.

…Dense layer

- Add proper dtype promotion in Dense.call()

- Add complex initialization and random generation support
@codecov-commenter
Copy link

codecov-commenter commented Jan 29, 2025

Codecov Report

Attention: Patch coverage is 72.72727% with 15 lines in your changes missing coverage. Please review.

Project coverage is 82.01%. Comparing base (9c8da1f) to head (d8ca6cd).
Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
keras/src/random/random.py 62.50% 7 Missing and 5 partials ⚠️
keras/api/_tf_keras/keras/random/__init__.py 0.00% 2 Missing ⚠️
keras/src/layers/core/dense.py 94.11% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master   #20823   +/-   ##
=======================================
  Coverage   82.01%   82.01%           
=======================================
  Files         559      559           
  Lines       52291    52348   +57     
  Branches     8084     8096   +12     
=======================================
+ Hits        42884    42931   +47     
- Misses       7431     7435    +4     
- Partials     1976     1982    +6     
Flag Coverage Δ
keras 81.82% <72.72%> (+<0.01%) ⬆️
keras-jax 64.29% <72.72%> (+0.02%) ⬆️
keras-numpy 59.03% <72.72%> (+0.03%) ⬆️
keras-openvino 29.83% <32.72%> (+<0.01%) ⬆️
keras-tensorflow 64.82% <72.72%> (+0.01%) ⬆️
keras-torch 64.28% <72.72%> (+0.09%) ⬆️

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.

@harshaljanjani
Copy link
Contributor Author

harshaljanjani commented Jan 29, 2025

Complex value support must be added for other layers as well. Please let me know if the changes made to Dense should be extrapolated to the other layers, as I intended this as a proof of concept. The issue will only be fully resolved when all layers support complex values.
Edit: I've tried to implement fixes solely dtypes.py without changing the layer implementation, but without fixing the initializer and the call() methods, it's tricky to implement it, since the previous PR (cited in the issue) seems to have already implemented the type promotion very well.

harshaljanjani and others added 2 commits January 29, 2025 20:14
TimeDistributedTest.test_basics previously failed due to incorrect dtype promotion, now passes with proper dtype handling
@harshaljanjani
Copy link
Contributor Author

Just as an aside, I faced torch-xla CI issues in this PR as well, which were resolved by merging keras-master as of January 30, 2024.

@harshaljanjani
Copy link
Contributor Author

Hi, just wanted to check if there's a reason this PR isn't moving forward. If the change is no longer needed or should be approached differently, please do let me know. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Assigned Reviewer
Development

Successfully merging this pull request may close these issues.

3 participants