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

Flax checkpoint updates #472

Merged
merged 36 commits into from
Dec 6, 2023
Merged

Flax checkpoint updates #472

merged 36 commits into from
Dec 6, 2023

Conversation

crstngc
Copy link
Contributor

@crstngc crstngc commented Nov 16, 2023

The focus of this PR is to enable Orbax checkpointing of Flax models and remove an obsolete dependency on Tensorflow.

Copy link

codecov bot commented Nov 16, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (021d904) 94.58% compared to head (68518e4) 94.80%.

Files Patch % Lines
scico/flax/train/state.py 76.92% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #472      +/-   ##
==========================================
+ Coverage   94.58%   94.80%   +0.22%     
==========================================
  Files          90       90              
  Lines        5608     5618      +10     
==========================================
+ Hits         5304     5326      +22     
+ Misses        304      292      -12     
Flag Coverage Δ
unittests 94.80% <96.34%> (+0.22%) ⬆️

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.

scico/flax/inverse.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@bwohlberg bwohlberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering the magnitude of the changes, it would be worth adding an entry in CHANGES.rst.

@bwohlberg
Copy link
Collaborator

Will this PR close #409?

@crstngc
Copy link
Contributor Author

crstngc commented Dec 1, 2023

Entries to CHANGES.rst were added. This PR should close #409.

Note that the adaptation of the CLU library implemented to display model parameters remains since the analogous functionality in Flax (based on nn.tabulate) is slightly more complex to deploy and does not include parameter statistics (mean and standard deviations).

@bwohlberg bwohlberg linked an issue Dec 1, 2023 that may be closed by this pull request
@bwohlberg bwohlberg self-requested a review December 5, 2023 16:28
Copy link
Collaborator

@bwohlberg bwohlberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only one minor issue remaining.

@bwohlberg bwohlberg merged commit e378d7a into main Dec 6, 2023
18 of 19 checks passed
@bwohlberg bwohlberg deleted the cristina/flax-updates branch December 6, 2023 00:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Flax Functionality
2 participants