Add Rician distribution #1387

mchitre · 2021-08-29T18:01:04Z

Rician distribution added as per #1290

devmotion

Thanks for the PR! I added some comments and suggestions 🙂

src/univariate/continuous/rician.jl

mchitre · 2021-08-30T10:29:30Z

@devmotion thanks for the comments. Will fix them after the parametrization approach is discussed and agreed on.

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

mchitre · 2021-08-30T13:14:39Z

@devmotion All comments addressed. Accepted changes are marked resolved. Other comments are left open for you to take a look at my changes and then to mark resolved. Thanks.

src/univariate/continuous/rician.jl

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

devmotion

The PR looks very good, thanks a lot! I just had some minor suggestions, if tests pass I think we can merge it 🙂

I opened a PR to StatsFuns that fixes the Float32 issues (JuliaStats/StatsFuns.jl#124), so we shouldn't worry about them in this PR.

src/univariate/continuous/rician.jl

test/truncate.jl

codecov-commenter · 2021-09-03T07:42:33Z

Codecov Report

Merging #1387 (99b74f9) into master (9ceb9a0) will increase coverage by 0.13%.
The diff coverage is 94.66%.

@@            Coverage Diff             @@
##           master    #1387      +/-   ##
==========================================
+ Coverage   82.86%   82.99%   +0.13%     
==========================================
  Files         116      117       +1     
  Lines        6676     6751      +75     
==========================================
+ Hits         5532     5603      +71     
- Misses       1144     1148       +4

Impacted Files	Coverage Δ
src/univariates.jl	`74.46% <ø> (ø)`
src/univariate/continuous/rician.jl	`94.66% <94.66%> (ø)`

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 9ceb9a0...99b74f9. Read the comment docs.

test/rician.jl

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

mchitre · 2021-09-05T09:17:41Z

But two still fail...

 MethodError: no method matching nchisqinvcdf(::Float64, ::Float64, ::Rational{Int64})
  Closest candidates are:
    nchisqinvcdf(::Union{Float64, Int64}, ::Union{Float64, Int64}, ::Union{Float64, Int64}) at /home/runner/.julia/packages/StatsFuns/vyFzQ/src/rmath.jl:89

because nchisqinvcdf doesn't support rationals.

To fix in StatsFuns too, or convert from rational to float while calling quantile on NoncentralChisq from Rician?

devmotion · 2021-09-05T10:00:44Z

I extended the StatsFuns PR, it covers Rationals as well now.

mchitre · 2021-09-09T16:22:01Z

@devmotion is there anything else needed from my end before we're ready to merge this?

devmotion · 2021-09-09T22:36:01Z

No, the PR looks great 👍 I just wanted to wait for JuliaStats/StatsFuns.jl#125, I still hope that it can be merged within the next days/week. If it takes longer, I think we can just mark the median tests as broken for now.

devmotion · 2021-09-30T22:19:00Z

Thanks for the PR, with StatsFuns 0.9.12 all tests pass now 🙂

Add Rician distribution

bf52934

mchitre mentioned this pull request Aug 29, 2021

Rician distribution #1290

Closed

devmotion reviewed Aug 30, 2021

View reviewed changes

mchitre and others added 10 commits August 30, 2021 19:47

Removed Rician Integer constructor based on review

9ddff52

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

Remove Rician inner constructor based on review suggestion

4ce1436

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

Removed @inline as suggested during review

e717bed

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

Type improvement based on review

c055087

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

Removed duplicate implementation for Rician pdf

cb43ef9

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

Removed Rician keyword constructor

e7f6b63

Merge branch 'rician' of github.com:mchitre/Distributions.jl into rician

bf9b995

Type stability improvments for Rician

4344ea1

Add back Rician Integer constructor to ensure tests pass

ac63f7b

Fixed Rician broken test cases

3121163

mchitre requested a review from devmotion August 30, 2021 13:14

devmotion reviewed Aug 30, 2021

View reviewed changes

src/univariate/continuous/rician.jl Outdated Show resolved Hide resolved

Updated Rician implementation to use NoncentralChisq

c222301

mchitre requested a review from devmotion August 31, 2021 15:57

devmotion reviewed Sep 1, 2021

View reviewed changes

mchitre and others added 7 commits September 2, 2021 01:25

Improve type stability in Rician

72bbe6d

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

Improve type stability in Rician

1fd4c6e

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

Improve type stability in Rician

a17e258

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

Improve type stability in Rician

55b2c79

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

Avoid type conversion in Rician

c1dced8

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

Improve type stability in Rician

397645d

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

Fixes edge cases without type instability in Rician

f8a7847

mchitre requested a review from devmotion September 1, 2021 17:51

devmotion mentioned this pull request Sep 3, 2021

Allow arguments of type Float16 and Float32 in R functions JuliaStats/StatsFuns.jl#124

Closed

devmotion reviewed Sep 3, 2021

View reviewed changes

test/rician.jl Outdated Show resolved Hide resolved

mchitre and others added 6 commits September 5, 2021 14:04

Use StatsFuns.sqrthalfπ in Rician

2ded077

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

Use StatsFuns.halfπ in Rician

ebadc70

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

Remove forcing of Float64 in Rician

8bc514a

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

Remove forcing of Float64 in Rician

dec17ca

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

Force same random numbers in Rician tests

05a9699

Increase tolerence on Rician random tests

8a0cada

devmotion mentioned this pull request Sep 13, 2021

Allow Real arguments in R functions and use Julia implementations only JuliaStats/StatsFuns.jl#125

Merged

Merge branch 'master' into rician

99b74f9

devmotion merged commit 9b98caa into JuliaStats:master Sep 30, 2021

mchitre deleted the rician branch October 1, 2021 07:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Rician distribution #1387

Add Rician distribution #1387

mchitre commented Aug 29, 2021

devmotion left a comment

mchitre commented Aug 30, 2021

mchitre commented Aug 30, 2021

devmotion left a comment

codecov-commenter commented Sep 3, 2021 •

edited

Loading

mchitre commented Sep 5, 2021 •

edited

Loading

devmotion commented Sep 5, 2021

mchitre commented Sep 9, 2021

devmotion commented Sep 9, 2021

devmotion commented Sep 30, 2021

Add Rician distribution #1387

Add Rician distribution #1387

Conversation

mchitre commented Aug 29, 2021

devmotion left a comment

Choose a reason for hiding this comment

mchitre commented Aug 30, 2021

mchitre commented Aug 30, 2021

devmotion left a comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 3, 2021 • edited Loading

Codecov Report

mchitre commented Sep 5, 2021 • edited Loading

devmotion commented Sep 5, 2021

mchitre commented Sep 9, 2021

devmotion commented Sep 9, 2021

devmotion commented Sep 30, 2021

codecov-commenter commented Sep 3, 2021 •

edited

Loading

mchitre commented Sep 5, 2021 •

edited

Loading