[REVIEW]: nardl:An R package to estimate the nonlinear cointegrating autoregressive distributed lag model

whedon commented 6 years ago

Submitting author: @zedtaha (zaghdoudi Taha) Repository: https://github.com/zedtaha/nardl Version: 0.1.3 Editor: @leeper Reviewer: @JoFAM Archive: Pending

Status

Status badge code:

HTML: <a href="http://joss.theoj.org/papers/fe9bdfe31cef65860883d6c7f8c0ced6"><img src="http://joss.theoj.org/papers/fe9bdfe31cef65860883d6c7f8c0ced6/status.svg"></a>
Markdown: [![status](http://joss.theoj.org/papers/fe9bdfe31cef65860883d6c7f8c0ced6/status.svg)](http://joss.theoj.org/papers/fe9bdfe31cef65860883d6c7f8c0ced6)

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) 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

@JoFAM, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

Make sure you're logged in to your GitHub account
Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

The reviewer guidelines are available here: https://joss.theoj.org/about#reviewer_guidelines. Any questions/concerns please let @leeper know.

Conflict of interest

[x] As the reviewer I confirm that I have read the JOSS conflict of interest policy and that there are no conflicts of interest for me to review this work.

Code of Conduct

[x] I confirm that I read and will adhere to the JOSS code of conduct.

General checks

[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Version: Does the release version given match the GitHub release (0.1.3)?
[x] Authorship: Has the submitting author (@zedtaha) made major contributions to the software? Does the full list of paper authors seem appropriate and complete?

Functionality

[x] Installation: Does installation proceed as outlined in the documentation?
[ ] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: If there are any performance claims of the software, have they been confirmed? (If there are no claims, please check off this item.)

Documentation

[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[ ] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g., API method documentation)?
[ ] Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
[ ] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

Software paper

[x] Authors: Does the paper.md file include a list of authors with their affiliations?
[ ] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[ ] References: Do all archival references that should have a DOI list one (e.g., papers, datasets, software)?

whedon commented 6 years ago

Hello human, I'm @whedon. I'm here to help you with some common editorial tasks. @JoFAM it looks like you're currently assigned as the reviewer for this paper :tada:.

:star: Important :star:

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿

To fix this do the following two things:

Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

For a list of things I can do to help you, just type:

@whedon commands

whedon commented 6 years ago

Attempting PDF compilation. Reticulating splines etc...

whedon commented 6 years ago

--> Check article proof :page_facing_up: <--

JoFAM commented 6 years ago

@whedon commands

whedon commented 6 years ago

Here are some things you can ask me to do:

# List all of Whedon's capabilities
@whedon commands

# Assign a GitHub user as the sole reviewer of this submission
@whedon assign @username as reviewer

# Add a GitHub user to the reviewers of this submission
@whedon add @username as reviewer

# Remove a GitHub user from the reviewers of this submission
@whedon remove @username as reviewer

# List of editor GitHub usernames
@whedon list editors

# List of reviewers together with programming language preferences and domain expertise
@whedon list reviewers

# Change editorial assignment
@whedon assign @username as editor

# Set the software archive DOI at the top of the issue e.g.
@whedon set 10.0000/zenodo.00000 as archive

# Open the review issue
@whedon start review

🚧 🚧 🚧 Experimental Whedon features 🚧 🚧 🚧

# Compile the paper
@whedon generate pdf

JoFAM commented 6 years ago

The package provides new functionality for R: nonlinear ARDL models currently have to be built manually or calculated using other software.

Current issues that warrant "major revision":

There is no guidelines for bug reports or contributions : https://github.com/zedtaha/nardl/issues/12
There is no testing done, results are impossible to check: https://github.com/zedtaha/nardl/issues/5
help files for the few exported functions don't contain return values etc. : https://github.com/zedtaha/nardl/issues/8

General remarks

In general the code is very ad-hoc and doesn't allow to easily extract test results for the different tests provided. None of the test functions are exported, so they can't be called directly. There's no real API to work with the S3 model object returned by nardl(). There's also no documentation as to what can be found in that object.

In order to be useful in a real world analysis, the API needs to be extended so one can at least work with the returned objects. In order to be maintainable, the code needs serious refactoring so it is more modularized.

@leeper : this is my assessment of the package nardl at its current state.

leeper commented 6 years ago

@JoFAM Thank you very much for your review! This is very useful feedback. Much appreciated.

@zedtaha I think the review does a very good job of identifying weaknesses in the structure of the code that you could address, particularly with regarding to making a fully featured API. Please take a look at the open issues and, especially, address https://github.com/zedtaha/nardl/issues/5. Thanks!

JoFAM commented 6 years ago

@zedtaha If you want, I can create a pull request from my fork, so you have already some of the issues (partly) taken care of. Also if you want further assistance with these and other issues, feel free to ping me. I'll be glad to help you clean up your already very valuable work.

zedtaha commented 6 years ago

@JoFAM ohh yes its a very good suggestion and I'm already OK and I found your comments very helpful.

JoFAM commented 6 years ago

@zedtaha Super. Do I create a pull request directly on your master branch, or do you want to create a devel branch first?

zedtaha commented 6 years ago

@JoFAM just make a pull and it will be OK

zedtaha commented 6 years ago

Hi @JoFAM i made some changes in the nardl package and i invite you to take a look

zedtaha commented 6 years ago

@JoFAM hope that the changes meet up all the issues ;)

JoFAM commented 6 years ago

@zedtaha Thx for the notice, I'll let you know. Give me a week though, it's pretty busy here.

leeper commented 6 years ago

Great, thanks @zedtaha!

zedtaha commented 6 years ago

Hi @JoFAM any things new?

JoFAM commented 6 years ago

@zedtaha sorry, I had a few family issues to take care of and didn't get to this yet. Thank you for reminding me, I am going to go through it again somewhere this week. Prob Thursday or Friday.

zedtaha commented 6 years ago

Hi @JoFAM any new!?

leeper commented 6 years ago

Hi @JoFAM, just a quick nudge.

JoFAM commented 6 years ago

@leeper @zedtaha I wrote you both a mail to explain the delay.

@zedtaha to help me get through it quicker, is it possible to link the issues to the commits where you solved them by mentioning them there? If you don't have the time, no worries, I'll hunt them down myself.

zedtaha commented 6 years ago

@leeper im in work so if you can do it'will be very grateful. Thank you a lot

JoFAM commented 6 years ago

@zedtaha I have another pull request that tackles a number of the issues from the previous review round:

https://github.com/zedtaha/nardl/pull/17

General remarks:

The package improved in this round, but still doesn't provide basic functionality one would expect. At least one should be able to extract coefficients and other important model statistics from a model object. They might need these to plot, to construct their own tests, to do simulation studies, ...

The documentation of the returned objects can be more helpful. Eg, it took me quite a while to realize 'np' is actually the number of lags, what 'sel' and 'fit' are and so forth.

There is much improvement possible in the unit testing. Currently the code coverage as reported by the covr package is about 14%. Even when the plot functions are ignored, there's not a single test for the pssbounds function or the summary.nardl function.

Points on which improvement is necessary

For me, the minimal requirements to give this package a pass, are the following :

Make sure pssbounds actually outputs something. Now the usefulness of this function is very limited: there's no easy way the results of that function can be captured and used in other code.
Add a few more unit tests for the important functions of the package. A code coverage of 13% is very low. Especially the pssbounds function with its 1700 lines really needs some testing.
Solve the failing unit test ( issue https://github.com/zedtaha/nardl/issues/15 ). That is actually a placeholder and example I added in the previous PR to get you started.
the output of the function nardl needs describing ( issue https://github.com/zedtaha/nardl/issues/8 )
You really need to give at least some indication of the source of the data ( issue https://github.com/zedtaha/nardl/issues/3 )

@leeper : This is my assesment of the package nardl at its current state.

JoFAM commented 6 years ago

@zedtaha PS: if you need extra explanation or more help with how to remediate my comments, don't hesitate to ask.

leeper commented 6 years ago

@JoFAM Thanks! This looks great. Really, really helpful review.

@zedtaha Can you incorporate the PR and try to address the remaining issues? Given the scale of @JoFAM's contribution via the PR, you may also want to list him in DESCRIPTION as a contributor.

zedtaha commented 6 years ago

Hi @JoFAM Thann you alot for your comments and I will try to do the needs. Dear @leeper I will do my best to resolve the issues as quickly as I can and thank you again.

leeper commented 6 years ago

@zedtaha Just wanted to check in. Are you still planning to make revisions for this?

leeper commented 6 years ago

@zedtaha Just another nudge to complete the requested revisions.

arfon commented 6 years ago

@zedtaha - friendly reminder to get to these revisions sometime soon please.

zedtaha commented 6 years ago

@leeper I apologize for the delay I was on a working trip. However, there is so many issues to do so I think its correct to withdrew the paper. Best

zedtaha commented 6 years ago

@leeper I'm also open to more suggestion.

JoFAM commented 6 years ago

@zedtaha Sad to see this disappear after all the work you've put in. If you need help later on when you want to update the package, feel free to tag me again.

Cheers Joris

arfon commented 5 years ago

@leeper I apologize for the delay I was on a working trip. However, there is so many issues to do so I think its correct to withdrew the paper. Best

Hi @zedtaha. Please feel free to resubmit this sometime again in the future if you're able to make the changes requested here by our reviewers.

@JoFAM @leeper, many thanks for all of your help here.

faheemja commented 5 years ago

i have a times series data matrix i want to fit functional autoregessive model of order one and two in R..how i can do do this in R???

danielskatz commented 5 years ago

@faheemja - this thread is was a review for a paper for the Journal of Open Source software, and is closed.

The repository for the software that was being reviewed, where you might want to ask your question, is https://github.com/zedtaha/nardl

openjournals / joss-reviews