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

Update analysis function #352

Closed
wants to merge 56 commits into from
Closed

Update analysis function #352

wants to merge 56 commits into from

Conversation

pengguanya
Copy link
Collaborator

@pengguanya pengguanya commented May 6, 2022

@gowerc

Functions modified

  • analysis
  • print.analysis
  • aconva
  • aconva_single

New functions added

  • analysis.R/analysis_result (exported)
  • analysis.R/as_analysis_result (exported)
  • analysis.R/is.analysis_result (exported)
  • analysis.R/namechecker
  • analysis.R/ana_name_chker
  • analysis.R/analysis_info
  • utilities.R/add_meta

A new 'analysis_result' class has been defined. Both built-in analysis function ancova and customer analysis function take input such as

 x <- list(
        analysis_result(
            name = 'trt1',
            est = coef(mod)[[group]],
            se = sqrt(vcov(mod)[group, group]),
            df = df.residual(mod),
            meta = add_meta('visit', ...)
        ),
        analysis_result(
            name = 'trt2',
            est = coef(mod)[[group]],
            se = sqrt(vcov(mod)[group, group]),
            df = df.residual(mod),
            meta = list(visit='1', something_else='c')
        ),
        analysis_result(
            name = 'trt3',
            est = coef(mod)[[group]],
            se = sqrt(vcov(mod)[group, group]),
            df = df.residual(mod)
            # meta is optional
        )
    )

The benefits of defining a new class to me are

  • Make it more explicit when user defining a analysis function
  • Make it possible to define methods and generic functions associated to this class (more flexible to add attraction and improve internal interfaces)
  • The base type of the class is implemented using list, therefore the lower level structure of the object does not change which leads to minimal impact to the internal interfaces

Various functions are implemented to validate the object, add meta information and extract information for analysis result. The analysis information are printed now in a tabular format with meta information in separate columns.

Tests have not been done yet. want to know your input and advice first. Thanks

analysis_info

@gowerc
Copy link
Collaborator

gowerc commented May 9, 2022

Hi @pengguanya ,

On the surface level this looks good, as in the input / output is roughly what I was hoping for. Only thing I can't make up my mind on is wether meta should be its own object or to just accept a base list. I think I am leaning towards having a list...

With regards to the internal implementation though I think it might need to be altered slightly, for a start we made an active decision to not use the tidyverse within this package to try and keep the dependency chain as light as possible so you will need to remove all uses of dplyr/purrr/rlang.

Minor point but in terms of consistency with the result of the package please can you replace stop_if_not with assert_that

@pengguanya
Copy link
Collaborator Author

@gowerc Thanks. I will make the changes and add test to each function for next pull request

As for now meta is a base list. The util function add_meta is only used to ease creation of meta for internal interfaces. It does return a base list and is not exported. So, for user, they need to do something like meta = list(...)

@pengguanya
Copy link
Collaborator Author

This pull request is deprecated. Please review pull request #362 255 02 instead

@gowerc gowerc closed this Jul 8, 2022
@cicdguy cicdguy deleted the 255-01 branch September 25, 2023 22:31
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