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

[Paper] add comparison tables and benchmark #127

Merged
merged 8 commits into from
Feb 10, 2024
Merged

Conversation

Santymax98
Copy link
Contributor

Hi Oskar, I don't know what you think about this, I used "BenchmarkTools.jl" to get the results. Can it be helpful for correction?

    Possible alternative to comparing the 3 packages
Copy link

codecov bot commented Feb 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b9bc7e4) 83.42% compared to head (5f1b355) 87.31%.
Report is 21 commits behind head on main.

❗ Current head 5f1b355 differs from pull request most recent head 2db2884. Consider uploading reports for the commit 2db2884 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #127      +/-   ##
==========================================
+ Coverage   83.42%   87.31%   +3.89%     
==========================================
  Files          29       29              
  Lines         748      749       +1     
==========================================
+ Hits          624      654      +30     
+ Misses        124       95      -29     

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

@lrnv
Copy link
Owner

lrnv commented Feb 6, 2024

First table looks very good. The second one is surprising to me, this is the first time I ran BenchmarkTools on these things, and i NEVER optimized the code, this is wild that we beat DataGenCopulaBased like that. What about BivariateCopulas why dont you have a result ?

@Santymax98
Copy link
Contributor Author

With "bivariatecopula.jl" it is only possible for the bivariate case and on the other hand I was not able to execute the code correctly. I was trying for quite some time using the documentation but I didn't succeed. Maybe it's a problem with my machine. On the other hand, do you think we should cite "BenchmarkTools" and put the characteristics of the machine I used to obtain those results?

add dates of package bivaratecopula
@lrnv
Copy link
Owner

lrnv commented Feb 7, 2024

That is good ! Yes you can cite benchmarktools like i cited other packages: with a link to their repository +, if they have some bibtex citation in their readme and/or docs, these bibtex entries. Look at what I did for BivariateCopulas to see how it works.

Nice numbers. The statement should be somthing like

Since our primary target is maintainability and readability of the implementation, we did not consider the efficiency and the perfromance of the code yet. However, a (limited in scope) benchmark on Copulas.jl's implementation of Clayton's pdf is availiable in Table 1 and shows competitve behavior of our implementation.

joss/paper.md Outdated
The following table shows some characteristics that differentiate each package.
| Characteristic | Copulas.jl | DatagenCopulaBased.jl | BivariateCopulas.jl |
|-----------------------------------------------|--------------------|--------------------|--------------------|
| Every Archimedean Copula sampling | Yes | No | No |
Copy link
Owner

Choose a reason for hiding this comment

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

You could add here a line "Classic Bivariate copulas sampling" with Yes/Yes/Yes.

Also a line "Obscure bivariate copulas sampling" with "Yes/No/No ?" ^^

Maybe the first column could be refactored to :
Sampling
- Classic Bivariate Copulas Yes Yes Yes
- Obscure Bivariate Copulas Yes No No
- Archimedean copulas All / Classic Only / Classic Only
- Multivariate Copulas Yes Yes No (You exchanged the two i think)
- Archimedean Chains (this is the real name of "nested" : No No Yes
Fitting No No Yes
Plotting No No Yes
Dependence metrics Partial / ? / ? (check)

@lrnv
Copy link
Owner

lrnv commented Feb 7, 2024

It would also be nice to add this benchmark to the documentation (with the code to run it) since it is pretty :)

@lrnv lrnv changed the title modified: joss/paper.md [Paper] add comparison tables and benchmark Feb 7, 2024
modifying tables, Benchmarktools.jl needs to be cited correctly
joss/paper.md Outdated
## Efficiency
To perform an efficiency test we use the "BenchmarkTools" package with the objective of comparing the execution time and the amount of memory necessary to generate copula samples with each package. We generate 10^6 samples for Clayton copula of dimensions 2, 5, 10 with parameter 0.8
To perform an efficiency test we use the [`BenchmarkTools.jl`](https://github.com/JuliaCI/BenchmarkTools.jl) [@BenchmarkTools] package with the objective of comparing the execution time and the amount of memory necessary to generate copula samples with each package. We generate 10^6 samples for Clayton copula of dimensions 2, 5, 10 with parameter 0.8
Copy link
Owner

@lrnv lrnv Feb 8, 2024

Choose a reason for hiding this comment

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

For the reference to work you need to add it to the joss/paper.bib file, taking it from their CITATION file there : https://github.com/JuliaCI/BenchmarkTools.jl/blob/master/CITATION.bib

@ARTICLE{BenchmarkTools,
  author =	 {{Chen}, Jiahao and {Revels}, Jarrett},
  title =	 "{Robust benchmarking in noisy environments}",
  journal =	 {arXiv e-prints},
  keywords =	 {Computer Science - Performance, 68N30, B.8.1, D.2.5},
  year =	 2016,
  month =	 "Aug",
  eid =		 {arXiv:1608.04295},
  archivePrefix ={arXiv},
  eprint =	 {1608.04295},
  primaryClass = {cs.PF},
  adsurl =	 {https://ui.adsabs.harvard.edu/abs/2016arXiv160804295C},
  adsnote =	 {Provided by the SAO/NASA Astrophysics Data System}
}

@Santymax98
Copy link
Contributor Author

Hello, I added the code, however I only added the one I used for copulas.jl but it works with any package. Additionally, it must be taken into account that the methodology for obtaining samples is different for each package.

@lrnv
Copy link
Owner

lrnv commented Feb 9, 2024

Thanks, this is Ok for now I will finsh this PR and merge it myself on monday. very good job.

Can you now focus on #98 from now on ? I am sure this is not very hard to solve but this needs to work correctly.

@Santymax98
Copy link
Contributor Author

Santymax98 commented Feb 9, 2024 via email

@lrnv
Copy link
Owner

lrnv commented Feb 9, 2024

Yes of course, add it under a section at the end of the paper before the references like so :

# Acknowledgement

Santiago Jiménez Ramos thanks XXX for the funding 

@Santymax98
Copy link
Contributor Author

Santymax98 commented Feb 9, 2024 via email

@lrnv lrnv merged commit 7cf9beb into lrnv:main Feb 10, 2024
3 of 5 checks passed
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.

[Paper] Add comparison table with BivariateCopulas.jl and DatagenCopulaBased.jl
2 participants