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

It/keras3 pytorch #396

Merged
merged 56 commits into from May 10, 2024
Merged

It/keras3 pytorch #396

merged 56 commits into from May 10, 2024

Conversation

IgorTatarnikov
Copy link
Member

@IgorTatarnikov IgorTatarnikov commented Apr 5, 2024

Before submitting a pull request (PR), please read the contributing guide.

Please fill out as much of this template as you can, but if you have any problems or questions, just leave a comment and we will help out :)

Description

What is this PR

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed?
Migrating away from tensorflow backend.

What does this PR do?
Makes torch the default backend, CI tests will only run with a torch backend on python 3.9, 3.10 and 3.11.

References

Please reference any existing issues/PRs that relate to this PR.

How has this PR been tested?

Tested locally and on CI.

Is this a breaking change?

Hopefully not? If people want to continue using a tensorflow installation all they have to do is run export KERAS_BACKEND="tensorflow" and make sure tensorflow>=2.16.1

Does this PR require an update to the documentation?

Not this one. The PR from cellfinder-to-keras-3 to main will definitely need extra documentation. Might be worth timing the release of the next version with a blog post explaining the changes.

Checklist:

  • The code has been tested locally
  • The code has been formatted with pre-commit

sfmig and others added 30 commits February 8, 2024 18:08
Co-authored-by: Igor Tatarnikov <61896994+IgorTatarnikov@users.noreply.github.com>
* Allow turning off classification or detection in GUI.

* Fix test.

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Refactor to fix code analysis errors.

* Ensure array is always 2d.

* Apply suggestions from code review

Co-authored-by: Igor Tatarnikov <61896994+IgorTatarnikov@users.noreply.github.com>

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Igor Tatarnikov <61896994+IgorTatarnikov@users.noreply.github.com>
matham and others added 5 commits May 1, 2024 11:13
* Support single z-stack tif file for input.

* Fix commit hook.

* Apply review suggestions.
* remove modular asv benchmarks

* recover old structure

* remove asv-specific lines from gitignore and manifest

* prune benchmarks
…ditional dependencies on M1 (#408)

* naive attempt at adapting to silicon mac CI

* run include guard test on Silicon CI

* double-check hdf5 is needed
* Replace coord map values with numba list/tuple for optim.

* Switch to fortran layout for faster update of last dim.

* Cache kernel.

* jit ball filter.

* Put z as first axis to speed z rolling (row-major memory).

* Unroll recursion (no perf impact either way).

* Parallelize cell cluster splitting.

* Parallelize walking for full images.

* Cleanup docs and pep8 etc.

* Add pre-commit fixes.

* Fix parallel always being selected and numba function 1st class warning.

* Run hook.

* Older python needs Union instead of |.

* Accept review suggestion.



* Address review changes.

* num_threads must be an int.

---------

Co-authored-by: Matt Einhorn <matt@einhorn.dev>
updates:
- [github.com/pre-commit/pre-commit-hooks: v4.5.0 → v4.6.0](pre-commit/pre-commit-hooks@v4.5.0...v4.6.0)
- [github.com/astral-sh/ruff-pre-commit: v0.3.5 → v0.4.3](astral-sh/ruff-pre-commit@v0.3.5...v0.4.3)
- [github.com/psf/black: 24.3.0 → 24.4.2](psf/black@24.3.0...24.4.2)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Copy link
Contributor

@sfmig sfmig left a comment

Choose a reason for hiding this comment

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

Looking good @IgorTatarnikov ! Mainly minor comments or questions in the review. Some higher-level comments below.

  • I know you say in the description that docs are not required atm, but maybe a quick edit to the README is useful so that we don't forget when we return to this. Could be just stating the need to install a backend, the need to configure the backend (via env variable for example) if developing, how to run tests locally with all backends etc.

  • I ran the tests locally using tox with the three backends and got some errors for torch that seem to relate to Apple chips. I wrote some more details and a possible workaround in the review.

  • The comparison with the previous performance looks great! Not the focus of this review, but I was thinking for the blogpost it would be more relevant to compare the torch implementation and the previous TF implementation more explicitly. Right now the comparison is more like: given an implementation, how does default model compare to retrained model.

.github/workflows/test_include_guard.yaml Show resolved Hide resolved
cellfinder/__init__.py Outdated Show resolved Hide resolved
cellfinder/__init__.py Outdated Show resolved Hide resolved
cellfinder/__init__.py Outdated Show resolved Hide resolved
cellfinder/__init__.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
IgorTatarnikov and others added 4 commits May 8, 2024 11:28
Co-authored-by: sfmig <33267254+sfmig@users.noreply.github.com>
* Simplify model download

* Update model cache
@IgorTatarnikov IgorTatarnikov merged commit 5a0152b into cellfinder-to-keras-3 May 10, 2024
17 checks passed
@IgorTatarnikov IgorTatarnikov deleted the it/keras3-pytorch branch May 10, 2024 09:09
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.

None yet

6 participants