Al-Murphy / MungeSumstats

Rapid standardisation and quality control of GWAS or QTL summary statistics
https://doi.org/doi:10.18129/B9.bioc.MungeSumstats
75 stars 16 forks source link

Fix bug in standardise_sumstats_column_headers_crossplatform #173

Closed cfbeuchel closed 8 months ago

cfbeuchel commented 8 months ago

Hi! Thanks for all the great work on the package! It's mind-boggling how messy working with GWAS sumstats is and your package is an enormous help. I've found a few things that could be improved in the function standardise_sumstats_column_headers_crossplatform.R, but this is, at least I think, simply a small bug that could be fixed. I'm sorry if it's wrong, but I am pretty sure (!cr %in% column_headers) makes no sense (cr is a column name) and the correct part of this if-statement should be !(cr %in% column_headers).

Commit message:

Fix a wrongly placed ! that should make sure a Corrected column name is not in the table. Was simply inside instead of outside of the brackets.

Al-Murphy commented 8 months ago

Thanks for this! Although I agree it does look a little odd, I think the placement of the ! doesn't make a difference here. See a test below for the two cases:

column_headers <- c("SNP","CHR","BP","A1","A2","FRQ","BETA","SE","P") 
#is there
cr = "FRQ"
testthat::expect_equal(!(cr %in% column_headers),(!cr %in% column_headers))
#isn't there
cr = "FRQ2"
testthat::expect_equal(!(cr %in% column_headers),(!cr %in% column_headers))

On either case, it gives the same result - is there an example you have where it does make a difference? If not, I'll just keep it as is even though it is a little odd looking!

cfbeuchel commented 8 months ago

Ah, wow, you are right! I guess that this is a specific feature for logical operations. Even though The ! is not valid as-is (which is what threw me off), in the context of a logical operation it is interpreted like a proper negation (A vs not A). So while not A in itself will not evaluate to anything, not A = B will evaluate to TRUE, since B is not A. Pretty cool!

> !"A"
Error in !"A" : invalid argument type
> !"A" == "B"
[1] TRUE

While in terms of semantics it is a little weird, it is completely correct. I'll close this PR then.