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

Refactor code #3

Merged
merged 39 commits into from
Sep 19, 2024
Merged

Refactor code #3

merged 39 commits into from
Sep 19, 2024

Conversation

jdeschamps
Copy link
Member

@jdeschamps jdeschamps commented Sep 16, 2024

Unfortunately, between the lack of time and me trying to understand the code, the refactoring has been performed on sight until a few commits ago where I finally formed an idea of what I wanted. The fact that everything moved to src layout in such a big PR, means that everything is basically new... Making reviewing the PR kinda difficult... Let's discuss it!

Here is a summary of the changes, sorted by categories.

Tooling

  • Use src layout (see reasons here)
  • Add pyproject.toml to hold the package metadata and packaging instructions.
  • Add missing dependencies to pyproject.toml, create dev optional dependencies.
  • Add additional pre-commit hooks (black for formatting, numpydoc for the doc).
  • Add CI to run tests on Github and publish to PyPi.
  • Add doctesting (sybil) to test the examples in the doc.

Testing and chore

  • Add tests to most functions.
  • Add type hints to all functions.
  • Add docstring to all functions.

Refactoring and changes

Main code base

  • Remove some of the unused code.
  • Rename methods for clarity (at least to me... debatable!)
  • During array linearization, use np.ravel to avoid unnecessary array copy (see MicroSSIM class).
  • Add raised errors in important functions (e.g. number of dims, type of inputs, gt vs pred shapes, etc.).
  • Replace the use of dictionaries by dataclasses to carry over SSIM elements and factors (SSIMElements and ScaledSSIM). This allows better static code checking and type inference, as well as being more readable.
  • Move methods into submodules (microssim.ssim, microssim.ri_factor, microssim.image_processing) and split into different files (e.g. ri_factor.py, mse_ri_factor.py). The final modules are kinda similar to what you had though, just with different names and in submodules.
  • Keep SSIM with C3 computation in the code base, but remove any call to it.
  • Refactor fitting of the scaling factor so that a single function is called (get_ri_factor).
  • Add skimage-like methods to run the metrics computation in a single call (micro_structural_similarity), this method is compatible with lists of arrays and arrays with a 3rd dimension.
  • Simplify MicroSSIM to make the internal calls simpler to follow, refactor some of the code into utility functions in other submodules (e.g. normalization, parameters computation).

Example notebook

  • Add missing functions to a local module (utils).
  • Refactor some of the plotting code into functions.
  • Refactor some code to make it more understandable.
  • Fixed all cells.

Others

  • Improve README.

@jdeschamps jdeschamps requested a review from ashesh-0 September 17, 2024 14:54
@jdeschamps jdeschamps marked this pull request as ready for review September 17, 2024 14:54
Copy link
Member

@ashesh-0 ashesh-0 left a comment

Choose a reason for hiding this comment

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

Looks good !

README.md Outdated
This is the official implementation of [MicroSSIM](https://arxiv.org/abs/2408.08747), accepted at BIC, ECCV 2024.
# MicroSSIM

MicroSSIM is an image metrics aimed at addressing the shortcominbs of the Structural
Copy link
Member

Choose a reason for hiding this comment

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

it is a "measure" and not a metric.
spelling of shortcoming

README.md Outdated
# MicroSSIM

MicroSSIM is an image metrics aimed at addressing the shortcominbs of the Structural
Similarity Index (SSIM), in particular in the context of microscopy images. Indeed,
Copy link
Member

Choose a reason for hiding this comment

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

M in SSIM is missing . M is for measure

@ashesh-0 ashesh-0 merged commit 1e05c8a into main Sep 19, 2024
14 checks passed
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.

2 participants