Skip to content

Conversation

johnnychen94
Copy link
Member

@johnnychen94 johnnychen94 commented Aug 3, 2021

By setting compatibility to ImageCore 0.9 only, there are a lot of legacy codes and compat patches being unnecessary now. This PR aims to clean the codebase.

TODO:

  • decide which deprecations need to be Base.depwarn(...; force=true)

closes #974
closes #976

@johnnychen94 johnnychen94 changed the title 🧹 legacy code organization and elimination 🧹 code organization and dead code elimination Aug 3, 2021
@codecov
Copy link

codecov bot commented Aug 3, 2021

Codecov Report

Merging #971 (972d810) into master (9796acc) will increase coverage by 0.98%.
The diff coverage is 78.68%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #971      +/-   ##
==========================================
+ Coverage   86.73%   87.72%   +0.98%     
==========================================
  Files           9        8       -1     
  Lines        1101      823     -278     
==========================================
- Hits          955      722     -233     
+ Misses        146      101      -45     
Impacted Files Coverage Δ
src/Images.jl 100.00% <ø> (+100.00%) ⬆️
src/corner.jl 88.40% <0.00%> (ø)
src/edge.jl 84.51% <40.00%> (ø)
src/deprecations.jl 87.08% <81.25%> (-0.77%) ⬇️
src/algorithms.jl 92.00% <100.00%> (+5.52%) ⬆️
src/compat.jl 50.00% <100.00%> (ø)
src/labeledarrays.jl 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9796acc...972d810. Read the comment docs.

@timholy
Copy link
Member

timholy commented Aug 28, 2021

It's quite satisfying to rip out large hunks of legacy code...thanks for starting this effort!

@johnnychen94
Copy link
Member Author

johnnychen94 commented Sep 1, 2021

At first, I only meant to clean some useless codes and upgrade to ImageCore 0.9, but it turns out that we're cleaning almost the entire src/algorithms.jl here:

  • var/std -> deprecated since ColorVectorSpace v0.9 (This is the only blocking issue for ImageCore v0.9 compatibility)
  • entropy -> maybe ImageBase (I have no idea what's the direct usage of this) (EDIT @timholy: ImageQualityIndices? It's a measure of the "variability" in intensity in an image, maximized by a uniform distribution of pixel intensities.)
  • div -> ImageBase and rename to fdiv finite divergence (corresponding to our new fdiff)
  • imgaussiannoise -> ImageNoise
  • imROF -> ImageNoise (reimplement imROF #755)
  • dilate/erode -> ImageMorphology (with requires.jl) (Transfer support for ImageMeta ImageMorphology.jl#44)
  • imaverage/imlaplacian -> deprecated with ImageFilterings.jl`
  • boxdiff/integral_image -> ? (is https://github.com/JuliaImages/IntegralArrays.jl a direct replacement?)
  • gaussian_pyramid -> maybe ImageFiltering with new name build_pyramid and supports generic kernels. (EDIT @timholy: I don't think we can, gaussian_pyramid requires imresize from ImageTransformations. This might be a good candidate for remaining in Images.jl.)
  • test_approx_eq_sigma_eps let's still keep it here for the moment until we have our test utils package ready.
  • blob_LoG/findlocalextrema/findlocalmaxima/findlocalminima -> ImageFiltering Move extremization, blob_LoG from Images ImageFiltering.jl#223
  • otsu_threshold/yen_threshold deprecate otsu_threshold and yen_threshold #897

timholy added a commit to JuliaGraphics/ColorVectorSpace.jl that referenced this pull request Sep 6, 2021
This is the complement to `varmult`. Useful for finishing
JuliaImages/Images.jl#971
@timholy
Copy link
Member

timholy commented Sep 6, 2021

The var/std fix relies on JuliaGraphics/ColorVectorSpace.jl#175

@johnnychen94
Copy link
Member Author

johnnychen94 commented Sep 7, 2021

I won't be available this week. I'll work on the imgaussiannoise , imROF and *_threshold for the next two weeks. For entropy, if we decide to move to ImageQualityIndexes, then we can do it like colorfulness so that we can allow a few more entropy implementations with shannon the default.

@timholy
Copy link
Member

timholy commented Sep 7, 2021

No rush. I think it will be better to wait for you and do the whole thing before making the next release. But if you disagree, I can get this stuff finished and make a 0.25 release, and then we can do the rest in 0.26.

timholy added a commit to JuliaImages/ImageMorphology.jl that referenced this pull request Sep 9, 2021
This came from Images.jl and is part of the code clean-out making
Images.jl mostly a "meta-package." It's also better to put glue
functionality in one of the packages that defines the components,
since that ensures that the glue-code is always available whenever
the operation can be supported.

xref JuliaImages/Images.jl#971
timholy added a commit to JuliaImages/ImageMorphology.jl that referenced this pull request Sep 9, 2021
This came from Images.jl and is part of the code clean-out making
Images.jl mostly a "meta-package." It's also better to put glue
functionality in one of the packages that defines the components,
since that ensures that the glue-code is always available whenever
the operation can be supported.

xref JuliaImages/Images.jl#971

This also deletes a schedule CI run, which risks disabling CI if activity is low.
@timholy
Copy link
Member

timholy commented Sep 9, 2021

OK, I think I've done all the ones you haven't carved out for yourself. I'm leaving div for you because it really seems to be part of imROF.

timholy added a commit to JuliaGraphics/ColorVectorSpace.jl that referenced this pull request Sep 9, 2021
This is the complement to `varmult`. Useful for finishing
JuliaImages/Images.jl#971
@johnnychen94
Copy link
Member Author

johnnychen94 commented Sep 10, 2021

For test_approx_eq_sigma_eps, there are two possible places to host it with enhancements: 1) https://github.com/JuliaPlots/VisualRegressionTests.jl already has a duplication so I believe people there would like to help maintain it, 2) separate https://github.com/JuliaTesting/ReferenceTests.jl into core package and image-specific packages so that we can add ImageFiltering dependency.

@timholy
Copy link
Member

timholy commented Sep 10, 2021

I'd be fine with either. We could also put it in ImageFiltering I guess?

@timholy
Copy link
Member

timholy commented Sep 10, 2021

OK if I rebase this? It turns out #927 had a breakage. But if you have local changes I don't want to make it harder for you.

@johnnychen94
Copy link
Member Author

Sure feel free to rebase

@timholy
Copy link
Member

timholy commented Oct 12, 2021

BTW, I know you are super-busy, so if you want to leave some of those items for later, that's fine. We could release a 0.25 and finish the rest in 0.26. Just wanted to let you know that it's not a contract!

@johnnychen94
Copy link
Member Author

I wanna make sure that we get a stable ImageBase version, introduce it to Images.jl, and then immediately deprecate their old versions. Otherwise, we would need some extra efforts to maintain the possible conflicts and compatibilities.

I'm working on ROF now, let me see if I can finish this this weekend, will this be okay?

@timholy
Copy link
Member

timholy commented Oct 12, 2021

Absolutely! I'm not in a rush, it's just that if it were to be, say, another couple months then maybe we'd want to think about an intermediate solution.

@johnnychen94 johnnychen94 force-pushed the jc/houseclean branch 2 times, most recently from 0ab55b8 to 8bff142 Compare November 3, 2021 00:23
@johnnychen94
Copy link
Member Author

johnnychen94 commented Nov 3, 2021

This is a long journey, finally, we're so close. Every commit is carefully checked to be clean so that if we get into trouble we can quickly identify the issues.

There are still some unchecked items in the todo-list but that doesn't block the new release. I thought we can get rid of the entire algorithm.jl file but turns out it takes more effort than I thought to modernize all the functions 😄

@timholy After this PR is merged we then need to update NEWS.md and the module docstring #982. I'll do this in the next few days.

timholy and others added 12 commits November 3, 2021 08:49
This commit introduces a new package IntegralArrays
This commit also removes old versions of ImageIO and ImageMagick
from compat list.

fixup
This requires ImageMorphology at least v0.3
This commit removes a lot of duplicated and lengthy import/export
lines so that we can focus on a more high-level management of this
umbrella package.

This requires at least Reexport v1.1 to work
This version removes two deprecated symbols: `ssim` and `psnr`.  This version
also reimplemented the entropy function using ImageContrastAdjustment so that
the function works on generic images.
- `accum` is removed
- `graytype` is made an internal trait
@timholy
Copy link
Member

timholy commented Nov 3, 2021

Thanks for all your efforts here. It is a ton of work, but the outcome feels like a major leap forward. 1.0 is starting to seem within sight...

@johnnychen94
Copy link
Member Author

If there's no plan to review this PR then I think we can merge it and prepare for the v0.25 release.

@johnnychen94 johnnychen94 merged commit feb7a00 into master Nov 24, 2021
@johnnychen94 johnnychen94 deleted the jc/houseclean branch November 24, 2021 15:53
johnnychen94 pushed a commit to JuliaImages/ImageMorphology.jl that referenced this pull request May 21, 2022
This came from Images.jl and is part of the code clean-out making
Images.jl mostly a "meta-package." It's also better to put glue
functionality in one of the packages that defines the components,
since that ensures that the glue-code is always available whenever
the operation can be supported.

xref JuliaImages/Images.jl#971

This also deletes a schedule CI run, which risks disabling CI if activity is low.
johnnychen94 pushed a commit to JuliaImages/ImageMorphology.jl that referenced this pull request May 21, 2022
This came from Images.jl and is part of the code clean-out making
Images.jl mostly a "meta-package." It's also better to put glue
functionality in one of the packages that defines the components,
since that ensures that the glue-code is always available whenever
the operation can be supported.

xref JuliaImages/Images.jl#971

This also deletes a schedule CI run, which risks disabling CI if activity is low.
johnnychen94 pushed a commit to JuliaImages/ImageMorphology.jl that referenced this pull request May 21, 2022
This came from Images.jl and is part of the code clean-out making
Images.jl mostly a "meta-package." It's also better to put glue
functionality in one of the packages that defines the components,
since that ensures that the glue-code is always available whenever
the operation can be supported.

xref JuliaImages/Images.jl#971

This also deletes a schedule CI run, which risks disabling CI if activity is low.
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