Closed editorialbot closed 7 months ago
Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.
For a list of things I can do to help you, just type:
@editorialbot commands
For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
@editorialbot generate pdf
Software report:
github.com/AlDanial/cloc v 1.88 T=0.06 s (1276.2 files/s, 94660.5 lines/s)
-------------------------------------------------------------------------------
Language files blank comment code
-------------------------------------------------------------------------------
Julia 46 410 194 2448
TeX 3 19 2 890
Markdown 21 350 0 806
TOML 3 136 1 593
YAML 7 1 7 134
CSS 1 0 0 17
-------------------------------------------------------------------------------
SUM: 81 916 204 4888
-------------------------------------------------------------------------------
gitinspector failed to run statistical information for the repository
Wordcount for paper.md
is 915
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.18637/jss.v021.i04 is OK
- 10.18637/jss.v034.i09 is OK
- 10.18637/jss.v039.i09 is OK
- 10.1201/9780367803896 is OK
- 10.1201/b18674 is OK
- 10.1214/07-AOS556 is OK
- 10.18637/jss.v098.i16 is OK
- 10.5281/zenodo.2647458 is OK
MISSING DOIs
- None
INVALID DOIs
- None
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
👋 @AnderGray, could you please update us on how it's going with your review?
Hi @osorensen and @lrnv :wave: , overall very nice work and I definitely recommend acceptance, here are some comments.
Overall the paper is clear and gives a good motivation to the package.
DataGenCopulaBased.jl
and BivariateCopulas.jl
, especially since both are registered in zenodo.Copulas.jl is clearly the must fully featured, and brings, as a key feature, the complience with the broader ecosystem.
it is not clear to me from tha paper alone that Copulas.jl
is the most feature complete of the three and would be good to give some more arguments in the paper. Ideally, a table comparing features of the three would be nice to have.
Overall the API is well documented and gives a lot of background knowledge. A few improvement suggestions
Examples
section with a few more examples of how to use the code, ideally within some applications of how to solve some "real-world" problems. E.g. applying Copulas.jl
to some popular datasets and concrete example problems.plot
, would be good to include the resulting image in the documentation. If possible, some visual representation would also be nice to have for the example of the READMEI spotted a few typos, you may consider using a tool like typos-cli to check more systematically. This could also be setup on CI (actually the julia repo does it) but it might be an overkill.
must -> most
complience -> compliance
specify -> specifies
folloiwng -> following
(I also think consists of
is the grammatically correct form, instead of is consisting of
but I am not native)exploit -> exploits
Exemples -> Examples
wtill -> still
The paper has some issues rendering some unicode characters
@lucaferranti Thanks a lot for your time and your review. I definitely agree with all the points you mentioned, 100%.
I will split your review into issues on Copulas.jl to be able to start working on each of them with my co-author @Santymax98. @osorensen it might take a while, but I think it is definitely worth it.
@editorialbot generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
@editorialbot generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
@osorensen @lrnv Hi 👋 you can expect my review this week. Overall, looks good
Thanks a lot for your reviews @lucaferranti and @AnderGray!
@editorialbot generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
@editorialbot generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
@editorialbot generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
@editorialbot generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
There we are; @lucaferranti we are almost done with your review, you can check out the list of tickets we have here : https://github.com/lrnv/Copulas.jl/issues/114 There are only two big tickets remaining, expected time one to two weeks. @osorensen is that acceptable timing ?
@lrnv looks very good! I think it's approaching convergence and looking forward to see teh final result after the fixes. I left an issue https://github.com/lrnv/Copulas.jl/issues/135 with a few minor improvement suggestions for the benchmarking table
oh in the comparison table of libraries there's a small typographic issue
but I am not sure if that can be fixed in any way in plain markdown, might have we just have to cope with it :/
@editorialbot generate pdf
@lucaferranti thanks, done. Markdown tables are indeed an obscure thing...
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
There we are; @lucaferranti we are almost done with your review, you can check out the list of tickets we have here : lrnv/Copulas.jl#114 There are only two big tickets remaining, expected time one to two weeks. @osorensen is that acceptable timing ?
Absolutely! It's also fine if it takes longer time, but in that case it would be great if you kept us updated on the progress.
Hi @lrnv and @osorensen! A very useful (and needed) tool in Julia. The documentation is also quite comprehensive and well written, and will serve as a good resource for those reading up or learning about copulas. Acceptance is certainly recommended.
This sentence could be confusing Copulas being fundamentally distributions of random vectors
. Maybe rephrase to something like: Since copulas can combine arbitrary distributions to form correlated random vectors …
Line 33 our code expressiveness and tidiness
-> our code’s …
Line 44 we did not consider …
-> we have not considered …
In the example 1., there is a bug. I think you want either x = rand(X,1000)
or D = SklarDist(C,(X1,X2,X3))
With the above fix, I still get an error, related to fitting the binomial distribution
ERROR: suffstats is not implemented for (Binomial, Vector{Float64}).
Stacktrace:
[1] error(s::String)
@ Base ./error.jl:35
[2] suffstats(dt::Type{Binomial}, xs::Vector{Float64})
@ Distributions ~/.julia/packages/Distributions/UaWBm/src/genericfit.jl:5
[3] fit_mle(dt::Type{Binomial}, x::Vector{Float64})
@ Distributions ~/.julia/packages/Distributions/UaWBm/src/genericfit.jl:27
[4] fit(dt::Type{Binomial}, x::Vector{Float64})
@ Distributions ~/.julia/packages/Distributions/UaWBm/src/genericfit.jl:33
[5] (::Copulas.var"#12#13"{Tuple{Gamma, Normal, Binomial}, Matrix{Float64}})(i::Int64)
@ Copulas ./cartesian.jl:0
[6] iterate
@ ./generator.jl:47 [inlined]
[7] collect_to!(dest::Vector{Distribution{Univariate, Continuous}}, itr::Base.Generator{UnitRange{Int64}, Copulas.var"#12#13"{Tuple{Gamma, Normal, Binomial}, Matrix{Float64}}}, offs::Int64, st::Int64)
@ Base ./array.jl:840
[8] collect_to!(dest::Vector{Gamma{Float64}}, itr::Base.Generator{UnitRange{Int64}, Copulas.var"#12#13"{Tuple{Gamma, Normal, Binomial}, Matrix{Float64}}}, offs::Int64, st::Int64)
@ Base ./array.jl:848
[9] collect_to_with_first!(dest::Vector{Gamma{Float64}}, v1::Gamma{Float64}, itr::Base.Generator{UnitRange{Int64}, Copulas.var"#12#13"{Tuple{Gamma, Normal, Binomial}, Matrix{Float64}}}, st::Int64)
@ Base ./array.jl:818
[10] collect(itr::Base.Generator{UnitRange{Int64}, Copulas.var"#12#13"{Tuple{Gamma, Normal, Binomial}, Matrix{Float64}}})
@ Base ./array.jl:792
[11] _totuple
@ ./tuple.jl:401 [inlined]
[12] Tuple
@ ./tuple.jl:369 [inlined]
[13] fit(#unused#::Type{SklarDist{FrankCopula, Tuple{Gamma, Normal, Binomial}}}, x::Matrix{Float64})
@ Copulas ~/Documents/julia/Copulas_review/Copulas.jl/src/SklarDist.jl:70
[14] top-level scope
@ REPL[10]:1
# Although you'll probbaly get a bad fit !
-> typo in probably
line 60 has seen
-> have seen
“# Add the loglikelyhood to the model
-> typo in loglikelihood
The Generator
example fails, perhaps you forgot to export?
ERROR: UndefVarError: `Generator` not defined
Stacktrace:
[1] top-level scope
@ REPL[5]:1
I agree with Luca that the docs could benefit from more code examples. Additionally, here’s a couple of typos I’ve found when reading through the docs:
There is a typo in the folder name: exemples
-> examples
for random ve tors and random variables
-> vectors
in https://github.com/lrnv/Copulas.jl/blob/6d5f5c5f3ca9724a2feba33612e404a0b0388a7e/docs/src/getting_started.md?plain=1#L102
In the first example, you say: Increase the number of observations to get a beter fit (or not?)
. Are you getting a bad fit because you've changed the distribution function Maybe it’s better to have a simpler example where you fit re-fit the Clayton
? Also beter
-> better
“SUite” -> “Suite”
Anticonomotony
-> anticomonotony
in https://github.com/lrnv/Copulas.jl/blob/6d5f5c5f3ca9724a2feba33612e404a0b0388a7e/docs/src/dependence_measures.md?plain=1#L36
The transformation, wich is called the inverse williamson transformation
-> which
and Williamson
in https://github.com/lrnv/Copulas.jl/blob/6d5f5c5f3ca9724a2feba33612e404a0b0388a7e/src/Generator/WilliamsonGenerator.jl#L14
Missing a close bracket in equation for InvGaussianGenerator in https://github.com/lrnv/Copulas.jl/blob/32d1ac56383f9da386f05d3992c6fe518e447125/src/Generator/UnivariateGenerator/InvGaussianGenerator.jl#L15
Copulas.jl
is very feature complete, including many different types of copulas (elliptical, empirical, Archimedean), and also allows you to create your own copulas using generators. The Williamson transformation is also a very nice idea! Although not required for acceptance of this paper/package, there's a couple suggested "good to have" features
Conditioning: in BivariateCopulas.jl
we perform conditioning of copulas and joint distributions using numerical finite differencing. Not sure if there's a better way to do it, but it allows you to sample from arbitrary (without a family) copulas and distributions. I think this is needed if you want to do implement vines
Rossenblat (and inverse) transformation is often used in uncertainty analysis (particularly reliability analysis) to transform random variables into independent standard normal variables.
Plotting / Visualisation
Thanks @AnderGray for your time, your review, and your ideas. I definitely agree with all of your comments and suggestions for Copulas.jl
. Let met first take a moment to move all your comments into separated issues on the repo, after what you will be able to track progress at this particular link.
@lucaferranti I notice that there is one unchecked item in your review checklist, related to example usage. Has this been addressed by @lrnv by now, or is there something more than should be done?
@osorensen this is still on my checklist but we are working on it. @lucaferranti I think this relates to https://github.com/lrnv/Copulas.jl/issues/117 ?
@AnderGray By the way, we are thinking with my coauthor that the feature requests you made in your review will take a long time to solve. They are clearly relevant and important, and we will keep them as issues to be solved in the future, but maybe they should not be blocking for the publication of the paper. What do you three @AnderGray @lucaferranti and @osorensen think about that ? I am talking specifically of issues https://github.com/lrnv/Copulas.jl/issues/150 https://github.com/lrnv/Copulas.jl/issues/151 and https://github.com/lrnv/Copulas.jl/issues/152
@lrnv Correct, those suggestions are not required for publication
@osorensen this is still on my checklist but we are working on it. @lucaferranti I think this relates to lrnv/Copulas.jl#117 ?
Correct the box I havent ticked yet is waiting for that issue.
Thanks to both of you for the quick response!
@osorensen all my comments have been addressed and the paper is ready from my side
Thanks @lucaferranti!
@editorialbot check references
@editorialbot generate pdf
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.18637/jss.v021.i04 is OK
- 10.18637/jss.v034.i09 is OK
- 10.18637/jss.v039.i09 is OK
- 10.1201/9780367803896 is OK
- 10.1201/b18674 is OK
- 10.1214/07-AOS556 is OK
- 10.18637/jss.v098.i16 is OK
- 10.5281/zenodo.2647458 is OK
- 10.5281/zenodo.7944064 is OK
- 10.5281/zenodo.10412898 is OK
MISSING DOIs
- None
INVALID DOIs
- None
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
@lrnv, as you note I have made a number of small issues and PRs which need to be fixed before we can move on with accepting the paper.
Anyway, congratulations with what seems to be an excellent package and a well-written paper.
@AnderGray Maybe you could help with lrnv/Copulas.jl#176 ?
@editorialbot set v0.1.21 as version
Done! version is now v0.1.21
@editorialbot set <DOI here> as archive
@editorialbot set <version here> as version
@editorialbot generate pdf
@editorialbot check references
and ask author(s) to update as needed@editorialbot recommend-accept
@lrnv, can you now please complete these tasks?
Submitting author: !--author-handle-->@lrnv<!--end-author-handle-- (Oskar Laverny) Repository: https://github.com/lrnv/Copulas.jl Branch with paper.md (empty if default branch): Version: v0.1.21 Editor: !--editor-->@osorensen<!--end-editor-- Reviewers: @lucaferranti, @AnderGray Archive: 10.5281/zenodo.6652672
Status
Status badge code:
Reviewers and authors:
Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)
Reviewer instructions & questions
@lucaferranti & @AnderGray, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @osorensen know.
✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨
Checklists
📝 Checklist for @lucaferranti
📝 Checklist for @AnderGray