-
Notifications
You must be signed in to change notification settings - Fork 141
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
ImageCorners.jl for corner algorithms #1028
Conversation
I'd flip this: leave |
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
|
The error comes from StatsBase. One possible |
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 |
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. |
Any time
If we can do a compatible version, sure, it's just making ImageEdgeDetection a dependency of Images and |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ReportPatch coverage has no change and project coverage change:
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
☔ View full report in Codecov by Sentry. |
Presumably this will become mergeable when we require Julia 1.6 (#1030) |
Closing and reopening to rerun tests |
Let's run tests again, and then is there any reason not to merge this? |
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