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

Use Number instead of Real for mgf and cf #1504

Closed
wants to merge 13 commits into from

Conversation

theogf
Copy link
Contributor

@theogf theogf commented Feb 2, 2022

As pointed out in #1502 , mgf and cf should be allowed to take Complex numbers and the second argument should be therefore relaxed.
All functions are also made type-stable.
Breaking?, when the argument for the mgf is given outside of the domain definition a DomainError is thrown.

Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

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

Thanks!

Unfortunately, there are a bunch of type stability issues. They exist already in the current implementation so it's probably beyond the scope of this PR to fix all them. I think we should just make sure to not introduce any new issues and possibly fix some simple cases where changes are needed anyway (but then we should also test them to make sure we don't reintroduce them accidentally...).

src/univariate/continuous/biweight.jl Outdated Show resolved Hide resolved
src/univariate/continuous/biweight.jl Outdated Show resolved Hide resolved
src/univariate/continuous/biweight.jl Outdated Show resolved Hide resolved
src/univariate/continuous/biweight.jl Outdated Show resolved Hide resolved
src/univariate/continuous/cauchy.jl Outdated Show resolved Hide resolved
src/univariate/discrete/discretenonparametric.jl Outdated Show resolved Hide resolved
src/univariate/discrete/discretenonparametric.jl Outdated Show resolved Hide resolved
src/univariate/discrete/discretenonparametric.jl Outdated Show resolved Hide resolved
src/univariate/discrete/negativebinomial.jl Outdated Show resolved Hide resolved
test/univariates.jl Outdated Show resolved Hide resolved
@theogf
Copy link
Contributor Author

theogf commented Feb 2, 2022

Thanks for the monster review! I tried to address your comments and integrate them. The inference tests should tell us where things go wrong :)

@theogf
Copy link
Contributor Author

theogf commented Feb 2, 2022

MGF are not necessarily defined on the whole Real/Complex domain, what should be returned when getting a value out of bound? NaN or ArgumentError ?

@devmotion
Copy link
Member

MGF are not necessarily defined on the whole Real/Complex domain, what should be returned when getting a value out of bound? NaN or ArgumentError ?

I was wondering the same. I'm a bit unsure, do you have a concrete example? Then it might be easier to see.

@codecov-commenter
Copy link

codecov-commenter commented Feb 10, 2022

Codecov Report

Merging #1504 (186066a) into master (620a73a) will increase coverage by 0.04%.
The diff coverage is 80.00%.

❗ Current head 186066a differs from pull request most recent head 9a1211d. Consider uploading reports for the commit 9a1211d to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1504      +/-   ##
==========================================
+ Coverage   84.92%   84.97%   +0.04%     
==========================================
  Files         128      128              
  Lines        7735     7765      +30     
==========================================
+ Hits         6569     6598      +29     
- Misses       1166     1167       +1     
Impacted Files Coverage Δ
src/Distributions.jl 100.00% <ø> (ø)
src/univariate/continuous/biweight.jl 61.76% <0.00%> (-1.88%) ⬇️
src/univariate/continuous/epanechnikov.jl 60.52% <0.00%> (-1.64%) ⬇️
src/univariate/continuous/triweight.jl 52.38% <0.00%> (-7.08%) ⬇️
src/univariate/continuous/cauchy.jl 85.10% <100.00%> (+0.66%) ⬆️
src/univariate/continuous/chisq.jl 76.66% <100.00%> (+1.66%) ⬆️
src/univariate/continuous/erlang.jl 69.44% <100.00%> (+1.79%) ⬆️
src/univariate/continuous/exponential.jl 94.00% <100.00%> (+0.25%) ⬆️
src/univariate/continuous/gamma.jl 94.38% <100.00%> (+0.12%) ⬆️
src/univariate/continuous/inversegamma.jl 96.00% <100.00%> (ø)
... and 26 more

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 620a73a...9a1211d. Read the comment docs.

@theogf
Copy link
Contributor Author

theogf commented Feb 17, 2024

This has gone very very stale so I will just close it.

@theogf theogf closed this Feb 17, 2024
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