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

Scorer rater #70

Merged
merged 19 commits into from
Nov 3, 2024
Merged

Scorer rater #70

merged 19 commits into from
Nov 3, 2024

Conversation

tomaszaba
Copy link
Collaborator

Hi Ernest,

This PR is about changes of a collection of functions that executes the plausibility check. For this, there are many files changed.
Changes made span from re-factoring, changing of function names, documentation and revising the test suite. **However, there is error detected by devtools::check().

What about the error?
devtools::check() detects errors in 37 tests. Apparently some functions are not being loaded with devtools::check() although they are listed in the NAMESPACE file. I tried to troubleshoot, but unsuccessfully. All those tests are well constructed and they pass with devtools::test() or devtools::test_active_file(). Also, all examples and vignettes work, including those using functions not detected with devtools::check(). Please give me a hand on this.

In terms of function organisation:

  • I split the plausibility check function into 3 files. Each files comes with 2 exported functions: the main plausibility check function and its companion to neat the output.
  • Everything else is in either quality_raters.R or quality_scorers.R. None of the functions in these are exported.

Function definition

  • All helpers come with an input check and respective test check in testthat. Regarding the tests, this time I make the use of regexp argument as I noticed that in my previous application (where I had not used this one), tests were passing not necessary for the exact reason why I expected it to pass.
  • I re-factored the plausibility check functions by removing the use of dplyr::group_by() inside the function definition. I know you laughed at me back in March/April when I first wrote that code. I changed it now 😅. Changes were also made in respective vignettes.

The amazing and nice looking mwana logo is now loaded as well. Please have a look if the placement is in line with the Nutriverse guidelines. I tried to follow the placement in the nipnTK package.

Thank you.

@tomaszaba tomaszaba added bug Something isn't working documentation Improvements or additions to documentation help wanted Extra attention is needed refactor code improvement/refactoring testing code testing labels Nov 3, 2024
Copy link
Member

@ernestguevarra ernestguevarra left a comment

Choose a reason for hiding this comment

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

@tomaszaba, see below for where your issues were arising from.

Please note also that I changed what you put in the README for the logo. nutriverse uses a very specific style for where the logo goes in the README file for GitHub. I will be very strict on this and will not compromise as this is nutriverse style.

You can merge this to dev and then the automated checks will activate for dev as well.

Comment on lines 9 to +12
library(testthat)
library(ipccheckr)
library(mwana)

test_check("ipccheckr")
test_check("mwana")
Copy link
Member

Choose a reason for hiding this comment

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

you forgot to change these.

Some of the checks passed because you had ipccheckr installed on your computer. But if PRs to dev had the ci/cd activated, you would have seen where the problem is. I have now activated ci/cd to run when doing a push or a PR to dev

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you Ernest.

@ernestguevarra ernestguevarra added this to the 3. Re-factor functions milestone Nov 3, 2024
@ernestguevarra ernestguevarra removed the help wanted Extra attention is needed label Nov 3, 2024
@tomaszaba
Copy link
Collaborator Author

@tomaszaba, see below for where your issues were arising from.

Please note also that I changed what you put in the README for the logo. nutriverse uses a very specific style for where the logo goes in the README file for GitHub. I will be very strict on this and will not compromise as this is nutriverse style.

You can merge this to dev and then the automated checks will activate for dev as well.

@ernestguevarra
That's super! Thanks. As for the logo, I am absolutely OK with that. As a matter of fact, I had mentioned in this PR that although I added it, I was not sure I fully followed the nutriverse guidelines. So super fine with me.

@ernestguevarra
Copy link
Member

  • I re-factored the plausibility check functions by removing the use of dplyr::group_by() inside the function definition. I know you laughed at me back in March/April when I first wrote that code. I changed it now 😅. Changes were also made in respective vignettes.

Just to be clear here. I didn't tell you to not use dplyr::group_by(). I told you not to use dplyr::ungroup() after a dplyr::summarise() because the behaviour in terms of output is a bit difficult to anticipate (in my opinion). I told you to use .groups = "drop" parameter inside dplyr::summarise() instead.

I also checked on the latest version of dplyr and the documentation for dplyr::summarise() still says that the .by and .groups argument in dplyr::summarise() is still experimental. It is possible that this argument will change or be removed or retained as stable in future versions. From a package building perspective, I will be very hesitant to use these arguments as they may change or be removed in future dplyr iterations. To account for this, you should specify a specific version of dplyr to have a dependency to in your package.

Merge branch 'dev' into ScorerRater

# Conflicts:
#	README.md
#	README.qmd
#	man/figures/README-workflow-1.png
#	man/figures/logo.png
@tomaszaba tomaszaba merged commit c0c5f23 into dev Nov 3, 2024
5 checks passed
@tomaszaba tomaszaba deleted the ScorerRater branch November 3, 2024 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation refactor code improvement/refactoring testing code testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants