Skip to content

Added varfinite#15

Merged
timholy merged 4 commits into
JuliaImages:masterfrom
jagoosw:varfinite
Sep 15, 2021
Merged

Added varfinite#15
timholy merged 4 commits into
JuliaImages:masterfrom
jagoosw:varfinite

Conversation

@jagoosw
Copy link
Copy Markdown
Contributor

@jagoosw jagoosw commented Sep 14, 2021

Reimplemented stdfinite suggested here as varfinite using the new style (which does look a lot better).

Checked and the new method is about twice as fast on a 50x60x70 array and allocates about 30% less memory.

I've also updated the tests for meanfinite, changing meanfinite(A, region) to meanfinite(A, dims=region), to stop the depreciation warnings during the tests. I've put these in a different commit in case these it was meant to be that way (apologies if that is the case).

Thanks!

I can't seem to find anything that has similar functionality
to meanfinite but for standard deviations and so have written
up what I've been using here.
Changed instances of `meanfinite(A, region)` to `meanfinite(A, dims=region)`
to stop the depreciation warnings during tests.
@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 14, 2021

Codecov Report

Merging #15 (2cdb62d) into master (9264ea2) will decrease coverage by 1.01%.
The diff coverage is 70.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #15      +/-   ##
==========================================
- Coverage   90.96%   89.94%   -1.02%     
==========================================
  Files           5        5              
  Lines         177      189      +12     
==========================================
+ Hits          161      170       +9     
- Misses         16       19       +3     
Impacted Files Coverage Δ
src/statistics.jl 81.48% <70.00%> (-6.76%) ⬇️
src/restrict.jl 96.82% <0.00%> (+0.05%) ⬆️

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 9264ea2...2cdb62d. Read the comment docs.

@jagoosw
Copy link
Copy Markdown
Contributor Author

jagoosw commented Sep 14, 2021

I've just copied over this fix for the 1.0 problem shared with meanfinite. I don't really understand the todo comment about replacing with Returns but would be happy to have a go at changing to that.

Copy link
Copy Markdown
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

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

This looks great, and with one small fix it should be good to go. You might also consider adding a test where n ends up being 1 in some entry, and checking that the returned value is also NaN.

With regards to Returns:

help?> Returns
search: Returns return

  f = Returns(value)

  Create a callable f such that f(args...; kw...) === value holds.

  Examples
  ≡≡≡≡≡≡≡≡≡≡

  julia> f = Returns(42);
  
  julia> f(1)
  42
  
  julia> f("hello", x=32)
  42
  
  julia> f.value
  42

  │ Julia 1.7
  │
  │  Returns requires at least Julia 1.7.

so we can't implement it until this package requires at least Julia 1.7 (or depends on Compat).

Comment thread test/statistics.jl Outdated
Test now checks for only one valid value (->\sigma=NaN).
@timholy timholy merged commit a8b8074 into JuliaImages:master Sep 15, 2021
@timholy
Copy link
Copy Markdown
Member

timholy commented Sep 15, 2021

Thanks for an excellent contribution! v0.1.4 is on the way: JuliaRegistries/General#44913

@jagoosw jagoosw deleted the varfinite branch September 15, 2021 14:17
@jagoosw
Copy link
Copy Markdown
Contributor Author

jagoosw commented Sep 15, 2021

Thanks for your help! I look forward to contributing more in the future

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