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

Add Moffat Kernel #147

Merged
merged 6 commits into from
Jan 27, 2020
Merged

Add Moffat Kernel #147

merged 6 commits into from
Jan 27, 2020

Conversation

gerrygralton
Copy link
Contributor

@gerrygralton gerrygralton commented Jan 21, 2020

In reference to feature request #146.
Please let me know if anything isn't done how it should be. I'm keen to learn.

src/kernel.jl Outdated Show resolved Hide resolved
src/kernel.jl Show resolved Hide resolved
src/kernel.jl Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jan 21, 2020

Codecov Report

Merging #147 into master will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #147      +/-   ##
==========================================
+ Coverage   91.59%   91.65%   +0.06%     
==========================================
  Files           9        9              
  Lines        1213     1222       +9     
==========================================
+ Hits         1111     1120       +9     
  Misses        102      102
Impacted Files Coverage Δ
src/ImageFiltering.jl 80% <ø> (ø) ⬆️
src/kernel.jl 100% <100%> (ø) ⬆️

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 46c0445...36c0a8a. Read the comment docs.

@johnnychen94 johnnychen94 merged commit 0e51025 into JuliaImages:master Jan 27, 2020
@timholy
Copy link
Member

timholy commented Jan 27, 2020

Thanks for the contribution, @gerrygralton, and to @johnnychen94 for getting this in!

β = rand()
@test Kernel.moffat(α,β,2) == Kernel.moffat(α,β,(2,2))

fwhm = ceil(Int, (α*2*sqrt(2^(1/β) - 1)) * 4)
Copy link
Member

Choose a reason for hiding this comment

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

There's nothing preventing β from being tiny here; this causes occasional test failures like this one:

moffat: Error During Test at /Users/travis/build/JuliaImages/ImageFiltering.jl/test/specialty.jl:220

  Got exception outside of a @test

  InexactError: trunc(Int64, 1.4650326325253482e37)

  Stacktrace:

   [1] trunc at ./float.jl:696 [inlined]

   [2] ceil(::Type{Int64}, ::Float64) at ./float.jl:357

   [3] macro expansion at /Users/travis/build/JuliaImages/ImageFiltering.jl/test/specialty.jl:225 [inlined]

   [4] macro expansion at /Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.0/Test/src/Test.jl:1083 [inlined]

   [5] macro expansion at /Users/travis/build/JuliaImages/ImageFiltering.jl/test/specialty.jl:221 [inlined]

   [6] macro expansion at /Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.0/Test/src/Test.jl:1083 [inlined]

   [7] top-level scope at /Users/travis/build/JuliaImages/ImageFiltering.jl/test/specialty.jl:6

   [8] include at ./boot.jl:317 [inlined]

   [9] include_relative(::Module, ::String) at ./loading.jl:1044

   [10] include(::Module, ::String) at ./sysimg.jl:29

   [11] include(::String) at ./client.jl:392

   [12] top-level scope at none:0

   [13] include at ./boot.jl:317 [inlined]

   [14] include_relative(::Module, ::String) at ./loading.jl:1044

   [15] include(::Module, ::String) at ./sysimg.jl:29

   [16] include(::String) at ./client.jl:392

   [17] top-level scope at none:0

   [18] eval(::Module, ::Any) at ./boot.jl:319

   [19] exec_options(::Base.JLOptions) at ./client.jl:243

   [20] _start() at ./client.jl:425

Test Summary: | Pass  Error  Total

specialty     |   98      1     99

  Laplacian   |   63            63

  gaussian    |   25            25

  DoG         |    6             6

  LoG         |    3             3

  moffat      |    1      1      2

While the fix seems obvious, @gerrygralton I'd rather leave this to you since you know the moffat stuff much better than I do.

Copy link
Member

Choose a reason for hiding this comment

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

And a different variant of the same theme:

moffat: Error During Test at /home/travis/build/JuliaImages/ImageFiltering.jl/test/specialty.jl:226

  Test threw exception

  Expression: Kernel.moffat(α, β) == Kernel.moffat(α, β, (fwhm, fwhm))

  OutOfMemoryError()

  Stacktrace:

   [1] Array at ./boot.jl:407 [inlined]

   [2] Array at ./boot.jl:415 [inlined]

   [3] Array at ./boot.jl:422 [inlined]

   [4] similar at /home/travis/.julia/packages/OffsetArrays/fZSaL/src/OffsetArrays.jl:124 [inlined]

   [5] similar at ./broadcast.jl:196 [inlined]

   [6] copy at ./broadcast.jl:840 [inlined]

   [7] materialize at ./broadcast.jl:820 [inlined]

   [8] moffat(::Float64, ::Float64, ::Tuple{Int64,Int64}) at /home/travis/build/JuliaImages/ImageFiltering.jl/src/kernel.jl:408

   [9] moffat at /home/travis/build/JuliaImages/ImageFiltering.jl/src/kernel.jl:410 [inlined]

   [10] moffat(::Float64, ::Float64) at /home/travis/build/JuliaImages/ImageFiltering.jl/src/kernel.jl:411

   [11] top-level scope at /home/travis/build/JuliaImages/ImageFiltering.jl/test/specialty.jl:226

   [12] top-level scope at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.5/Test/src/Test.jl:1113

   [13] top-level scope at /home/travis/build/JuliaImages/ImageFiltering.jl/test/specialty.jl:221

   [14] top-level scope at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.5/Test/src/Test.jl:1113

   [15] top-level scope at /home/travis/build/JuliaImages/ImageFiltering.jl/test/specialty.jl:6

@gerrygralton gerrygralton mentioned this pull request Mar 6, 2020
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.

3 participants