ImageCorners.jl for corner algorithms

[ashwani-rathee]

• Deprecate corner algorithms in Images.jl base and use ImageCorners.jl

I am bit doubtful about if I am doing the deprecation the right way. Also seemingly unexpected error happen for me in tests for Exposure

[timholy]

I'd flip this: leave tests/corner.jl (you want to make sure those tests still work after the deprecation, that's the test of whether this is neutral with respect to users), but don't add all the corner functions to src/deprecations.jl (because they're already provided by @reexporting ImageCorners).

[ashwani-rathee]

One problem is perhaps me exposing Percentile in ImageCorners.jl which is also here in Images.jl, it seems to resolve those errors showing in tests of corner.jl. I am not sure yet why the others are showing up:

Test Summary:              | Pass  Error  Total     Time
Images                     |  831      8    839  3m47.9s
  ColorizedArray           |   51            51     3.2s
  Algorithms               |  110           110    38.7s
  Exposure                 |   50      4     54    19.2s
    Histogram              |           1      1     3.7s
    Histogram Equalisation |           1      1     0.4s
    Gamma Correction       |   38            38     8.4s
    Histogram Matching     |           1      1     0.4s
    CLAHE                  |           1      1     2.4s
    Other                  |   12            12     4.0s
  Edge                     |  306           306    37.9s
  Corner                   |  108      4    112     1.0s
    Corners API            |   23            23     0.0s
    Corners Sub-pixel API  |   46      1     47     0.6s
    Harris                 |           1      1     0.1s
    Shi-Tomasi             |           1      1     0.1s
    Kitchen-Rosenfeld      |           1      1     0.2s
    Fast Corners           |   39            39     0.0s
  show (MIME)              |   30            30    16.1s
  legacy                   |   91            91  1m02.0s
  deprecations             |   85            85    12.4s

[timholy]

Also seemingly unexpected error happen for me in tests for Exposure

The error comes from StatsBase. status --outdated -m revealed that we needed JuliaImages/ImageSegmentation.jl#93 before we could upgrade. So I fixed the error in that ImageSegmentation PR and registered a new version. Once that is out, you can bump StatsBase to 0.34 and push as another commit to re-run the tests.

One possible [compat]-based failure would be Julia 1.3, but we can address that once we see any errors.

[timholy]

One problem is perhaps me exposing Percentile in ImageCorners.jl which is also here in Images.jl

I have to step out and don't have time to look at the code, so I'm not 100% sure I know exactly what you mean. If struct Percentile is defined in two places then you should fix that (delete the struct definition from here and use the one in ImageCorners.jl). But you also have to make sure that all the appropriate promote_rule definitions are in the same package where the struct definition occurs (it would be piracy otherwise).

[ashwani-rathee]

I'll update when ImageSegmentation.jl new version is out. About Percentile, it's used by algorithms that are still in Images.jl( in src/edge.jl in canny) and ImageCorners.jl both 😅. Can we deprecate canny too in this PR given it's already provided by ImageEdgeDetection.jl? Which also raises the issue of inclusion of ImageEdgeDetection.jl here(and removal of edge.jl) but it becomes quite far fetched given it seems like a lot of restructuring in ImageEdgeDetection.jl in terms of API.

[timholy]

Any time PkgA say using PkgB, it has the full resources of exports from PkgB. So as long as Images depends on ImageCorners and ImageCorners exports Percentile, there is absolutely no reason to redefine it in Images. Indeed, if you do, then (1) ImageCorners can't export it, and (2) users of JuliaImages have to work with two incompatible versions of Percentile (you couldn't pass ImageCorners.Percetile(0.8) to an algorithm defines in Images, or vice versa). Whereas if Images just imports the ImageCorners definition, then there is only one Percentile and everything works seamlessly. You're basically just moving code from one directory to another. Whatever the "lowest level" package that needs it should define it and all others get it via using.

Can we deprecate canny too in this PR given it's already provided by ImageEdgeDetection.jl? Which also raises the issue of inclusion of ImageEdgeDetection.jl here(and removal of edge.jl) but it becomes quite far fetched given it seems like a lot of restructuring in ImageEdgeDetection.jl in terms of API.

If we can do a compatible version, sure, it's just making ImageEdgeDetection a dependency of Images and @reexporting it. But if that's hard, let's wait until we're planning the next breaking release. In an ideal world, we would add a deprecation warning. E.g., [email protected] could add a warning saying that v0.26 will switch to using ImageEdgeDetection.canny and show how to convert to the new API. That way anyone who runs tests (which make depwarns visible) will see a message telling them how to rewrite their code. I always appreciate that kind of help myself, so it's nice to provide it to others when possible.

[ashwani-rathee]

I think the exposure-related should have gotten fixed after I merged the changes from master, yet they error out and as you had mentioned, 1.3 gives compat issue.

[timholy]

The test failures are due to an ancient version of StatsBase being installed: in the CI log you can see that v0.30 was installed, but the current version is v0.34. You can see why if you run

pkg> status --outdated -m

If this doesn't show it being held back at v0.30, then it's probably something in the test environment, so do this:

using TestEnv
TestEnv.activate()    # while the Images environment is active
]status --outdated -m

and then that will show you the constraints due to the test environment.

[codecov]

Codecov Report

Patch coverage has no change and project coverage change: -0.49 ⚠️

Comparison is base (1069fd4) 87.72% compared to head (083c835) 87.24%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1028      +/-   ##
==========================================
- Coverage   87.72%   87.24%   -0.49%     
==========================================
  Files           8        6       -2     
  Lines         823      674     -149     
==========================================
- Hits          722      588     -134     
+ Misses        101       86      -15     

Impacted Files	Coverage Δ
src/Images.jl	100.00% <ø> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[timholy]

Presumably this will become mergeable when we require Julia 1.6 (#1030)

[timholy]

Closing and reopening to rerun tests

[timholy]

Let's run tests again, and then is there any reason not to merge this?