Conversation
This revealed some bugs in the untested bivariate T cdf. I've fixed the bugs and also added a few tests.
| # The source call the commented out line below. Instead we error out | ||
| throw(DomainError(nu, "degrees of freedom parameter must be positive")) | ||
| # return bvnuppercdf(-dh, -dk, r) |
There was a problem hiding this comment.
That is, in tvpack.f this branch is supported? And we error since bvnuppercdf does not exist/was not copied?
There was a problem hiding this comment.
We do have bvnuppercdf but I think it's weird behavior to call it when the degrees of freedom parameter is non-positive.
There was a problem hiding this comment.
I see and I agree, it seems strange to support non-positive degrees. Maybe in tvpack it is just a way to encode the limit case of infinte degrees of freedom?
In any case, shouldn't tcdf be changed as well for consistency? That supports non-positive degrees of freedom as well it seems.
There was a problem hiding this comment.
It should. I hadn't noticed that. Will push an update.
There was a problem hiding this comment.
Reading this again. The branch is for nu < 1. I'd read it as < 0. It doesn't makes sense at all to use the normal cdf when the degrees of freedom parameter is between zero and one. Not sure what the idea here was.
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
Codecov ReportBase: 59.85% // Head: 75.34% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #148 +/- ##
===========================================
+ Coverage 59.85% 75.34% +15.48%
===========================================
Files 14 21 +7
Lines 568 653 +85
===========================================
+ Hits 340 492 +152
+ Misses 228 161 -67
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
It seems this PR is almost ready? Only https://github.com/JuliaStats/StatsFuns.jl/pull/148/files#r1019101826 is missing, it seems? |
|
I'm not completely sure what to do with the issue mentioned in #148 (comment) |
| if VERSION >= v"1.7" | ||
| using JET | ||
| @testset "Static analysis" begin | ||
| @test isempty(JET.get_reports(report_package("StatsFuns", target_modules=(StatsFuns,)))) |
There was a problem hiding this comment.
One could also use JET.test_package, e.g.,
| @test isempty(JET.get_reports(report_package("StatsFuns", target_modules=(StatsFuns,)))) | |
| JET.test_package("StatsFuns", target_defined_modules=true) |
There was a problem hiding this comment.
Or
| @test isempty(JET.get_reports(report_package("StatsFuns", target_modules=(StatsFuns,)))) | |
| JET.test_package("StatsFuns", target_defined_modules=true) |
3 participants
This revealed some bugs in the untested bivariate T cdf. I've fixed the bugs and also added a few tests.