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

Review karim #123

Closed
wants to merge 6 commits into from
Closed

Review karim #123

wants to merge 6 commits into from

Conversation

Karim-Mane
Copy link
Member

@Karim-Mane Karim-Mane commented Dec 11, 2023

@jamesmbaazam below are some comments, suggetions and actions from the review I performed

  1. I noticed that some files names are .r and others are .R. Is there any specific reason for this? If not, maybe consider harmonising the file naming.

  2. I have updated the .lintr file by copy-pasting from package template

  3. the example from the simulate_summary() function returns a vector of Inf values. Is this intentional? If yes, would not it be more informative to provide an example that returns actual data?

  4. The example from the likelihood() function is not working. This is because the chains_likelihood list only contains Inf values. Maybe consider using a different example.

  5. In borel.r:

    • no examples for the exported functions
    • you are using return() in the other functions but not in rborel(). Consider harmonising on return().

Base automatically changed from review to main December 15, 2023 20:07
@jamesmbaazam
Copy link
Member

Thanks for the review, Karim. I have made several changes from #121 including ones that intersect with your comments. I will link the issues and PR's in my replies below and close this PR. You will be added as a reviewer when #198 is closed.

@jamesmbaazam below are some comments, suggetions and actions from the review I performed

  1. I noticed that some files names are .r and others are .R. Is there any specific reason for this? If not, maybe consider harmonising the file naming.

No, there is no reason. I most of the files with .r are legacy files and the newer ones one .R extension. I will fix this in #135.

  1. I have updated the .lintr file by copy-pasting from package template

Thanks for this. I have already fixed it in #180.

  1. the example from the simulate_summary() function returns a vector of Inf values. Is this intentional? If yes, would not it be more informative to provide an example that returns actual data?

Indeed, it will be more informative. I will fix it in #150 and #151.

  1. The example from the likelihood() function is not working. This is because the chains_likelihood list only contains Inf values. Maybe consider using a different example.

Thanks. This has been fixed in main.

  1. In borel.r:
  • you are using return() in the other functions but not in rborel(). Consider harmonising on return().

I have merged a PR that ensures that all functions have explicit returns. See #190.

@jamesmbaazam jamesmbaazam deleted the review_karim branch January 30, 2024 16:36
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.

3 participants