search

spgarbet / tangram

Table Grammar package for R
66 stars 3 forks source link

Test statistic not showing any longer #48

Closed kylerove closed 4 years ago

kylerove commented 5 years ago

I don't know when this changed, but its been a few months since I ran my code. Now it is not showing the test statistic column. I tried with no transform, hmisc, my own transform. What am I missing?

`table1 <- tangram(group ~ age[2]

spgarbet commented 5 years ago

There has been some debate and back and forth about the test statistic present / absence. Add test=TRUE as an argument and I think that will work.

Other things, the id is now pulled from the R chuck id if possible if you're using Rmarkdown. Also if you just call it in a chuck (i.e. drop assignment and later render call) it will autodetect if it's being called from Rmarkdown and find the right renderer to use, so a document will compile to either PDF or HTML with no code changes.

I've also added documentation for the transforms, for example ?hmisc will show you the arguments that get passed in.

kylerove commented 5 years ago

Sweet thank you! I tend to write my analyses and output independent results (tables, figures). Just haven't found a good resource to get into rmarkdown workflow. Would love to integrate with versioning/git. Again, this package is awesome.

I will have to write my own function for test statistics....have a project that uses propensity matching. Will want matching-appropriate comparisons. All the best!

spgarbet commented 5 years ago

I want to add more transforms. I will write the guts of it if you provide me an Rmd file showing the table layout you desire, the types to compare. I do have one currently that was contributed smd that is in the package that allows for propensity weighting. Might want to check that transform out. Anyway, send me something that generates test data, some example of the test and a sketch and I can turn around a repeatable transform for you and integrate it into the package. Remember to define all comparisons desired that are sensible (e.g. Numeric X Cat, Cat X Numeric, Numeric X Numeric, Cat X Cat) and I'll list you as a co-author.

kylerove commented 4 years ago

Boom. Finally got around to it. Here is Rmd: https://github.com/kylerove/tangram_psm

Everything appears to work well and runs without errors, but the tables won't display because:

Error in table_flatten(x) : 
  (list) object cannot be coerced to type 'logical'
In addition: Warning message:
In any(sapply(table, function(y) sapply(y, function(z) inherits(z,  :
  coercing argument of type 'list' to logical

This probably has something to do with the table-building code. Anyway, let me know what you think. I hope we can get this working soon!

spgarbet commented 4 years ago

That warning is interesting. It's saying the the type detection is failing and it's trying to coerce a list to logical. Should be a minor tweak.

spgarbet commented 4 years ago

This is odd, because sex and lang work individually and technically should have no interaction in the code as it pastes them together after running separately. This means that some global variable is leaking between the two.

> tangram(group ~ sex, data=m1.final, pref_test = "wilcox.test", block=m1.final$block, test=TRUE, transform = psm)
===================================================
     N        0             1        Test Statistic
            (N=30)        (N=30)                   
---------------------------------------------------
sex  60  22 (73.333%)  22 (73.333%)        —       
===================================================
Numerical summary is median (IQR). Categorical is N (%). ^1 Logistic regression with GEE. ^2 Cochran Mantel Maenszel.Warning message:
In tangram.formula(group ~ sex, data = m1.final, pref_test = "wilcox.test",  :
  tangram() will require unique id to be specified in the future
> tangram(group ~ lang, data=m1.final, pref_test = "wilcox.test", block=m1.final$block, test=TRUE, transform = psm)
======================================================
      N        0             1        Test Statistic  
------------------------------------------------------
             (N=30)        (N=30)                     
lang  60                                  0.630       
   1      12 (40.000%)  8 (26.667%)                   
   2      6 (20.000%)   7 (23.333%)                   
   3      6 (20.000%)   10 (33.333%)                  
   4      6 (20.000%)   5 (16.667%)                   
======================================================
Numerical summary is median (IQR). Categorical is N (%). ^1 Logistic regression with GEE. ^2 Cochran Mantel Maenszel.Warning message:
In tangram.formula(group ~ lang, data = m1.final, pref_test = "wilcox.test",  :
  tangram() will require unique id to be specified in the future
> tangram(group ~ sex+lang, data=m1.final, pref_test = "wilcox.test", block=m1.final$block, test=TRUE, transform = psm)
Error in table_flatten(x) : 
  (list) object cannot be coerced to type 'logical'
In addition: Warning messages:
1: In tangram.formula(group ~ sex + lang, data = m1.final, pref_test = "wilcox.test",  :
  tangram() will require unique id to be specified in the future
2: In any(sapply(table, function(y) sapply(y, function(z) inherits(z,  :
  coercing argument of type 'list' to logical
kylerove commented 4 years ago

Yeah, I'm not super adept at debugging tools in R. I usually resort to print() statements to narrow down lines that might be generating the issue. This one though, stumped me because no actual error comes out until you try to display the result.

spgarbet commented 4 years ago

Okay, there's a lot to unpack on this one. First the fix is easy.

Change line 424 in Matched cohort stats from add_col to add_row.

So the error message you got was because the sub-tables didn't line up. I had the same problem and fixed it in the original you copied from.

I see several things that should happen because of this: 1) The error message needs to be descriptive to triage the problem faster. This was a really simple problem once it was obvious what it was. However logical conversion issues are nonsensical. So I'm going to add much better error handling that checks for mis-specified transforms. 2) There is a huge amount of boiler plate code copied over to meet your end goal. It would be far better that only the necessary overrides were required, making this use case much simpler to implement. However, kudos for diving into the deep end. I will work on turning this into a much simpler method that makes the same tables. 3) I'd like to include this use-case as another available transform for users. Do I have your permission to use this code and include you as a contributing author?

kylerove commented 4 years ago

Please feel free to use the code and modify/simplify as needed to meet the end goals. I figured it was related to the table generation but couldn’t see the issue. Nice work!

On Sep 20, 2019, at 10:06 AM, Shawn Garbett notifications@github.com wrote:

 Okay, there's a lot to unpack on this one. First the fix is easy.

Change line 424 in Matched cohort stats from add_col to add_row.

So the error message you got was because the sub-tables didn't line up. I had the same problem and fixed it in the original you copied from.

I see several things that should happen because of this:

The error message needs to be descriptive to triage the problem faster. This was a really simple problem once it was obvious what it was. However logical conversion issues are nonsensical. So I'm going to add much better error handling that checks for miss specified transforms. There is a huge amount of boiler plate code copied over to meet your end goal. It would be far better that only the necessary overrides were required, making this use case much simpler to implement. However, kudos for diving into the deep end. I will work on turning this into a much simpler method that makes the same tables. I'd like to include this use-case as another available transform for users. Do I have your permission to use this code and include you as a contributing author? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

kylerove commented 4 years ago

I made an error in my mapping of the various comparison contingencies (numerical x categorical, etc). I didn't realize it. Because the only valid options for comparison should be numerical x cat and cat x cat, I have removed the other contingency options within psm() in my .rmd example.

The function summarize_numerical() is still generating the table improperly and generating an error. I've played around with it for a few hours, but can't find the issue which is frustrating. :/

Anyway, look forward to integrating this transform into your package.

spgarbet commented 4 years ago

I found a simple fix to prevent this issue from being a problem. Going to close this bug ticket and open an enhancement for the other issues.