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

Issue https://github.com/statOmics/msqrob2/issues/63 #64

Merged
merged 13 commits into from
Sep 3, 2024

Conversation

leopoldguyot
Copy link

Cf. issue:
#63

R/msqrob.R Show resolved Hide resolved
##### None exported functions from multcomp package is included here to
##### During R and Bioc checks

propagateFalseStatus <- function(vectors, statuses) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice attempt to solve the interaction issue, I however can't fully wrap my head around what you are doing. A few comments on the function:

  • Could you document the internal function using standard comments so to briefly describe what the function does, and especially what are the expected argument inputs.
  • Be very careful with repeat to not allow for infinite loops, I personally prefer a while() loop although there is still is risk of infinite loops.
  • I can't see unit tests, but maybe this is in progress

Copy link
Author

Choose a reason for hiding this comment

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

I added tests, commented the function and changed the code. a97a4e5

@cvanderaa
Copy link
Collaborator

cvanderaa commented Aug 29, 2024

Let us know when your PR is done, I will then pull your branch and get my hands on your code to see if I can break it through some additional unit tests 😈 😉

@leopoldguyot leopoldguyot changed the base branch from master to issue63 August 30, 2024 11:28
@cvanderaa cvanderaa merged commit 285af81 into statOmics:issue63 Sep 3, 2024
@cvanderaa
Copy link
Collaborator

This marks the end of Léopold's internship. The branch issue63 is still work in progress, to be discussed in #63.

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.

2 participants