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.

Sign up for GitHub

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

Jump to bottom

bug fix for fredman posthoc #485

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

bug fix for fredman posthoc #485

wants to merge 1 commit into from

Conversation

boxizhang
Copy link
Contributor

Title: Debug of the Friedman posthoc tests

Problem: When the data is not sorted by tested variable and subject, there is a chance that the paired Wilcoxon test will not be performed properly

Solution: sort the data based on tested variable and subject.

Key Features:

  • Key feature 1
  • Key feature 2
  • ...

Checklist

  • Make sure you are requesting to pull a feature/bugfix branch (right side). This should not be main or develop.
  • Make sure you are make a pull request against either main or develop (left side). (Requesting to main should be reserved for bugfixes and new releases)
  • Add or update unit tests (if applicable)
  • Check your code with any unit tests (Run devtools::check() locally)
  • Add neccessary documentation (if applicable)

Type of changes

What type of changes does your code introduce?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue) (link the issue on the right)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Code style update (formatting, renaming)
  • Documentation Update
  • Other (explain)

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, any questions you have, etc...

Consider linking any issues (#issue-number ) or adding @mentions to ensure specific people see it.

kathy-nevola
kathy-nevola reviewed Dec 6, 2024
View reviewed changes
Copy link
Contributor

@kathy-nevola kathy-nevola left a comment

Choose a reason for hiding this comment

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

There were a few parts I wasnt sure about, see my questions. Happy to schedule a meeting if that would be easier.

OlinkAnalyze/R/Olink_one_non_parametric.R
@@ -311,6 +313,7 @@ olink_one_non_parametric_posthoc <- function(df,
olinkid_list = NULL,
variable,
test = "kruskal",
subject = "Subject",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this default to Null and expect that there is no subject data

OlinkAnalyze/R/Olink_one_non_parametric.R
@@ -259,6 +260,7 @@ olink_one_non_parametric <- function(df,
#' @param olinkid_list Character vector of OlinkID's on which to perform post hoc analysis. If not specified, all assays in df are used.
#' @param variable Single character value or character array.
#' @param test Single character value indicates running the post hoc test for friedman or kruskal.
#' @param subject Single character value indicates running the post hoc test for friedman or kruskal.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the name of a column or just a string. If there are options we should specify them in the documentation

OlinkAnalyze/R/Olink_one_non_parametric.R
@@ -259,6 +260,7 @@ olink_one_non_parametric <- function(df,
#' @param olinkid_list Character vector of OlinkID's on which to perform post hoc analysis. If not specified, all assays in df are used.
#' @param variable Single character value or character array.
#' @param test Single character value indicates running the post hoc test for friedman or kruskal.
Copy link
Contributor

Choose a reason for hiding this comment

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

add strings for tests

OlinkAnalyze/R/Olink_one_non_parametric.R
dplyr::select(!!!rlang::syms(subject)) %>%
dplyr::pull() %>%
unique())){
message(paste("Subjects removed due to incomplete data"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Add list of subject IDs?

Copy link
Contributor

Choose a reason for hiding this comment

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

Are subjects only removed from specific assays?

OlinkAnalyze/R/Olink_one_non_parametric.R
dplyr::mutate(Friedman_remove = "no") %>%
dplyr::select(-n)

if(length(subject_remove %>%
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the Friedman test not work with subjects that are missing timepoints?

OlinkAnalyze/R/Olink_one_non_parametric.R
' has only NA:s in atleast one level of ',
effect,
'. It will not be tested.'),
call. = F)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
call. = F)
call. = FALSE)

OlinkAnalyze/R/Olink_one_non_parametric.R
}
}

formula_string <- paste0(data_type, "~", paste(variable,collapse="*"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the formula include subject as well? Like in the LME? (Unsure how this model using the formula string)

OlinkAnalyze/R/Olink_one_non_parametric.R
unique() %>%
pull() %>%
length()

Copy link
Contributor

Choose a reason for hiding this comment

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

n_subject -> number of levels in a variable?

OlinkAnalyze/R/Olink_one_non_parametric.R
dplyr::filter(n == n_subject) %>%
dplyr::mutate(Friedman_remove = "no") %>%
dplyr::select(-n)

Copy link
Contributor

Choose a reason for hiding this comment

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

subject_remove -> subject-Assay combos that are not missing levels of the variable

OlinkAnalyze/tests/testthat/data/refResults.R
@@ -41,6 +41,7 @@ kruskal_posthoc_results <- olink_one_non_parametric_posthoc(npx_data1,
friedman_posthoc_results <- olink_one_non_parametric_posthoc(npx_data1,
variable = "Time",
test = "friedman",
subject = "Subject",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a unit test to test for this case?

This was linked to issues Jan 16, 2025
olink_one_non_parametric and missing timepoints in paired-data #345
Closed
multiple testing correction in olink_one_non_parametric_posthoc outside the group_by #346
Open
@kathy-nevola kathy-nevola removed a link to an issue Jan 16, 2025
olink_one_non_parametric and missing timepoints in paired-data #345
Closed
@kathy-nevola kathy-nevola linked an issue Jan 16, 2025 that may be closed by this pull request
[FEATURE] Allow subject argument to olink_one_non_parametric_posthoc when using Friedman's test #478
Open
@kathy-nevola
Copy link
Contributor

Can we also address Issue #346 in this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kathy-nevola kathy-nevola kathy-nevola left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@boxizhang @kathy-nevola