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

Raftery copula #91

Merged
merged 15 commits into from
Nov 30, 2023
Merged

Raftery copula #91

merged 15 commits into from
Nov 30, 2023

Conversation

Santymax98
Copy link
Contributor

I added the multivariate Raftery copula and some tests. I did not implement Kendall's tau because it is a very long equation (its form is closed) however it is very long. If you think it is necessary, you could implement it. I leave you the main reference
[https://www.mdpi.com/2227-7390/11/2/414]

but you can also see details in Nelsen 2006.

Santymax98 and others added 5 commits November 17, 2023 00:19
	new file:   test/test_plotly.jl
	deleted:    src/Plotly_functions.jl
	new file:   test/RafteryTest.jl
	deleted:    test/test_plotly.jl
	modified:   src/Copulas.jl
	modified:   src/MiscellaneousCopulas/RafteryCopula.jl
Copy link
Owner

@lrnv lrnv left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this one, I did not even know this copula !

Overall this looks good, I gave a few comments on the code, if you could go fix these that would be very nice. Also a test of the correctness of the pdf and cdf would be nice.

There is something that tickles me : the current state of your code, especially the R.d error, is showing that you did not run your own tests before sending this code. How are you working with this package ? I suspect that you are complicating your own life by not using the standard workflow, maybe we could discuss that together if you want ? Basically you should be doing the following :

1° In some julia environnement, type ] dev Copulas into the repl to developp the package.
2° Open ~/.julia/dev/Copulas.jl or the location of the package, setup this location to use my repo as upstream and your repo as origin
3° Make branches inside this repository directly and work there.

This will allow you to :

  1. Use directly your changes in any other julia environement by typing ]dev Copulas, especially usefull if you use Revise
  2. Run the tests directly into your developping environement by ] test or using the vscode interface.

If you need some help with that i can help you

src/MiscellaneousCopulas/RafteryCopula.jl Outdated Show resolved Hide resolved
src/MiscellaneousCopulas/RafteryCopula.jl Outdated Show resolved Hide resolved
src/MiscellaneousCopulas/RafteryCopula.jl Outdated Show resolved Hide resolved
src/MiscellaneousCopulas/RafteryCopula.jl Outdated Show resolved Hide resolved
src/MiscellaneousCopulas/RafteryCopula.jl Show resolved Hide resolved
src/MiscellaneousCopulas/RafteryCopula.jl Outdated Show resolved Hide resolved
test/RafteryTest.jl Outdated Show resolved Hide resolved
@Santymax98
Copy link
Contributor Author

Santymax98 commented Nov 30, 2023 via email

@lrnv
Copy link
Owner

lrnv commented Nov 30, 2023

No need to appologise, this is normal you have to learn :)

Maybe the begining of this video could help you : https://www.youtube.com/watch?v=qM9NtiYlXck especially the Revise.jl part, if you dont use it yet. the rest of the video is taling about other stuffs.

@Santymax98
Copy link
Contributor Author

Santymax98 commented Nov 30, 2023 via email

@lrnv
Copy link
Owner

lrnv commented Nov 30, 2023

Yes kendall tau would be nice !
The fitting procedure from the article would be nice too !

@Santymax98
Copy link
Contributor Author

Santymax98 commented Nov 30, 2023 via email

@Santymax98
Copy link
Contributor Author

The bibtex is next
@Article{Raftery2023,
title={Multivariate extension of Raftery copula},
author={Saali, Tariq and Mesfioui, Mhamed and Shabri, Ani},
journal={Mathematics},
volume={11},
number={2},
pages={414},
year={2023},
publisher={MDPI}
}

However, I don't know why the assert folder does not appear in my branch.

I think I implemented the changes. Regarding the adjustment. It is simply a maximization of the likelihood function. However, when I use Distributions.fit it asks me for sufficient statistics and when I wanted to use optim to implement it independently it won't let me because of the dependencies.

@lrnv
Copy link
Owner

lrnv commented Nov 30, 2023

So I merged the main branch into yours, so that there is no more conflicts. Dont forget to pull the modifications or you will erase them !

For the fitting procedure, if it is just a non-linear maximization of the LLH, then let's not implement it. We should implement that somewhere else as a generic, I agree.

Copy link
Owner

@lrnv lrnv left a comment

Choose a reason for hiding this comment

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

Can you see the docs/src/assets/references.bib now ?

Still a few tests but nothing too big i think, this is really good !

src/MiscellaneousCopulas/RafteryCopula.jl Outdated Show resolved Hide resolved
Copy link

codecov bot commented Nov 30, 2023

Codecov Report

Attention: 21 lines in your changes are missing coverage. Please review.

Comparison is base (dc47f47) 80.70% compared to head (1920d01) 79.14%.

Files Patch % Lines
src/MiscellaneousCopulas/RafteryCopula.jl 58.00% 21 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #91      +/-   ##
==========================================
- Coverage   80.70%   79.14%   -1.56%     
==========================================
  Files          28       29       +1     
  Lines         679      729      +50     
==========================================
+ Hits          548      577      +29     
- Misses        131      152      +21     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

term_numerator = 1 - d - R.θ * u_ordered[d]^((1 - R.θ - d) / (1 - R.θ))
term_product = prod(u)^((R.θ) / (1 - R.θ))

logpdf_value = log(term_numerator) - log(term_denominator) + log(term_product)
Copy link
Owner

@lrnv lrnv Nov 30, 2023

Choose a reason for hiding this comment

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

The pdf was not ran by the tests. Could you add a test to check if the pdf is, at least, not producing errors ? If possible, could you add another one that checks that the values produced are the right ones ? Maybe by computing one or two values by hand for specific parameters in dimension two ?

In particular, I am worried that term_denominator and term_numerator are negative, which will make the log produce an error. adding these tests is a very good way of knowing if it is ok or not :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to do it now

@lrnv
Copy link
Owner

lrnv commented Nov 30, 2023

Perfect ! Let's merge that then, congratulation ! I will tag a new release to push it into the registry

@lrnv lrnv merged commit ec3d0ee into lrnv:main Nov 30, 2023
7 checks passed
@Santymax98
Copy link
Contributor Author

Santymax98 commented Nov 30, 2023 via email

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