keras icon indicating copy to clipboard operation
keras copied to clipboard

Fixes for reference before assignment…

Open SamuelMarks opened this issue 1 year ago • 17 comments

found through static analysis tooling

(more to go; or you can merge now and await further PRs; or you can request I move this to one PR per file or per module)

SamuelMarks avatar Oct 17 '23 18:10 SamuelMarks

Thanks for the PR. Unit tests are failing and the value-add of most of these changes is highly questionable (most of it is unreachable code which will only serve to decrease our coverage). Please double check each change and only keep the useful ones.

fchollet avatar Oct 20 '23 08:10 fchollet

@fchollet Hmm ok I'll see what's causing these segfaults. In the meantime a lot of the patches are to solve the traceability problem, i.e., guarantee that the output variable is constructed as per below:

-        for output in ds.take(1):
-            output = output.numpy()
+        output = next(iter(ds)).numpy()

Should I change the PR to just that fix, and make separate PR(s) for the other cases?

SamuelMarks avatar Oct 25 '23 18:10 SamuelMarks

Hi @SamuelMarks Can you please resolve conflicts? Thank you!

gbaned avatar Dec 01 '23 08:12 gbaned

@gbaned - Sure thing. Done.

SamuelMarks avatar Dec 06 '23 15:12 SamuelMarks

Codecov Report

Attention: Patch coverage is 55.55556% with 8 lines in your changes missing coverage. Please review.

Project coverage is 76.31%. Comparing base (11be99a) to head (2978b7a).

Files Patch % Lines
keras/src/applications/densenet.py 0.00% 2 Missing :warning:
keras/src/models/model.py 33.33% 2 Missing :warning:
keras/src/backend/torch/trainer.py 0.00% 1 Missing :warning:
keras/src/layers/attention/attention.py 0.00% 1 Missing :warning:
keras/src/legacy/backend.py 0.00% 1 Missing :warning:
keras/src/optimizers/base_optimizer.py 0.00% 1 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #18640      +/-   ##
==========================================
- Coverage   78.83%   76.31%   -2.53%     
==========================================
  Files         498      498              
  Lines       45864    45874      +10     
  Branches     8450     8446       -4     
==========================================
- Hits        36159    35009    -1150     
- Misses       7999     9053    +1054     
- Partials     1706     1812     +106     
Flag Coverage Δ
keras 76.24% <55.55%> (-2.46%) :arrow_down:
keras-jax 65.32% <55.55%> (+2.94%) :arrow_up:
keras-numpy 57.10% <44.44%> (+0.15%) :arrow_up:
keras-tensorflow 66.61% <50.00%> (+2.94%) :arrow_up:
keras-torch ?
keras.applications 80.33% <0.00%> (?)
keras.applications-jax 80.33% <0.00%> (?)
keras.applications-numpy 22.93% <0.00%> (?)
keras.applications-tensorflow 80.33% <0.00%> (?)
keras.applications-torch 80.06% <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 Dec 06 '23 16:12 codecov-commenter

Hi @SamuelMarks Can you please resolve conflicts? Thank you!

gbaned avatar Dec 29 '23 09:12 gbaned

@gbaned Done

SamuelMarks avatar Dec 29 '23 16:12 SamuelMarks

@SamuelMarks , Can you please rebase the code to the latest code structure, eg: keras/layers is now keras/src/layers

sachinprasadhs avatar May 01 '24 22:05 sachinprasadhs

@sachinprasadhs No problem. Merged and resolved conflicts.

SamuelMarks avatar May 02 '24 04:05 SamuelMarks

Hi @SamuelMarks Can you please resolve conflicts? Thank you!

gbaned avatar Jun 14 '24 06:06 gbaned

@gbaned Done.

SamuelMarks avatar Jun 14 '24 17:06 SamuelMarks

Hi @SamuelMarks Still conflicts are appearing, can you please resolve those? Thank you!

gbaned avatar Jun 17 '24 04:06 gbaned

@gbaned Done

SamuelMarks avatar Jun 21 '24 04:06 SamuelMarks