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

Bivariate Plackett Copula Addition #40

Merged
merged 8 commits into from
Oct 26, 2023
Merged

Conversation

Santymax98
Copy link
Contributor

The file PlackettCopula.jl is added that includes: an instance of the bivariate Plackett Copula, calculation of the CDF and PDF functions, the random sample generator and the Spearman Rho calculation. Additionally, I added some testing these functions in biv_plackett.jl.

Santymax98 and others added 6 commits October 26, 2023 00:56
… bivariate Plackett Copula, calculation of the CDF and PDF functions, the random sample generator and the Spearman Rho calculation. Additionally, I added some testing these functions in biv_plackett.jl.
… bivariate Plackett Copula, calculation of the CDF and PDF functions, the random sample generator and the Spearman Rho calculation. Additionally, I added some testing these functions in biv_plackett.jl.
@lrnv
Copy link
Owner

lrnv commented Oct 26, 2023

Hey,

Thanks a lot for this proposal ! I changed a few things to comply with the current standard, but globally everything is working great. I did commit to your branch so please pull down from it before doing anything more.

One thing : Your tests are not passing on the pdf and the cdf, because julia is computing values that are more precise than what you asked for, maybe you should choose is you want to round them or something so that the tests pass ?

Moreover, where are these values coming from ? the reference ? You could write that as a comment in the test file.

I am not sure about what to do with the Base.rand() and Distributions._rand!() situation, it seems like from a copula to the other there is no clear standard, I should investigate that. But this is not a problem for this PR.

Correct your tests, and then we can move on to other models :)

@Santymax98
Copy link
Contributor Author

Santymax98 commented Oct 26, 2023 via email

@lrnv
Copy link
Owner

lrnv commented Oct 26, 2023

I fixed the tests so that they pass, now we could merge this.

If you want to add more to this PlackettCopula, you can still change things. Otherwise, tell me so and I'll merge this pull request.

@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

Merging #40 (9494de8) into main (810ebb9) will increase coverage by 3.68%.
The diff coverage is 94.28%.

@@            Coverage Diff             @@
##             main      #40      +/-   ##
==========================================
+ Coverage   67.70%   71.38%   +3.68%     
==========================================
  Files          21       22       +1     
  Lines         322      360      +38     
==========================================
+ Hits          218      257      +39     
+ Misses        104      103       -1     
Files Coverage Δ
src/Copulas.jl 100.00% <ø> (ø)
src/MiscellaneousCopulas/PlackettCopula.jl 94.28% <94.28%> (ø)

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Santymax98
Copy link
Contributor Author

Santymax98 commented Oct 26, 2023 via email

@lrnv
Copy link
Owner

lrnv commented Oct 26, 2023

Well, This is probably the easiest thing to do yes, to create a new PR. You might add a few of them at once if you want. This PR interface allows us to discuss and collaborate easily on the code, dont you think ?

I'll merge and close this one now, thanks a lot !

Btw, Congratulation on you first merged pull request !

@lrnv lrnv merged commit 9a0b803 into lrnv:main Oct 26, 2023
@Santymax98 Santymax98 deleted the PlackettCopula branch October 26, 2023 15:44
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