search

neurogenomics / MAGMA_Celltyping

Find causal cell-types underlying complex trait genetics
https://neurogenomics.github.io/MAGMA_Celltyping
71 stars 31 forks source link

Increase test coverage #95

Closed bschilder closed 1 year ago

bschilder commented 2 years ago

Currently quite low. Made extra tricky since test-map_snps_to_genes takes a long time. Not ideal for a unit test, but might be worth un-hashing. Screenshot 2022-01-07 at 14 58 20

NathanSkene commented 2 years ago

Somewhat offtopic, but can't we delete the format_sumstats functions now we've got MungeSumstats? That would presumably increase the code coverage

bschilder commented 2 years ago

I think that's actually a really good idea @NathanSkene . I was trying to avoid back-compatibility issues, but given that we're bumping to the version to 2.0.0 i think this is an ideal time to completely remove these functions. This will def help to increase coverage.

Al-Murphy commented 2 years ago

I don't really agree - I think making things as easy as possible for a user like telling them that a function is no longer in use is more important than the code coverage percentage. My notes on code coverage was that a lot of functions weren't being tested and secondly that I thought testing should be done to ensure the results from this version are the same as using the old version. I don't think we will be able to or should be aiming to get the code coverage percentage up to a very high percent and should instead focus on what we are actually testing. Plus we could just add a test where these defunct functions are called then they would be covered anyway

bschilder commented 2 years ago

I don't really agree - I think making things as easy as possible for a user like telling them that a function is no longer in use is more important than the code coverage percentage.

You have a point here, it's something I've been going back and forth on. Perhaps worth a brief discussion during lab meeting?

Plus we could just add a test where these defunct functions are called then they would be covered anyway

I'd agree except that some of these functions introduce the requirement for genome build Bioc packages, which take a while to install. Just something to consider.

Al-Murphy commented 2 years ago

You have a point here, it's something I've been going back and forth on. Perhaps worth a brief discussion during lab meeting?

Sure!

I'd agree except that some of these functions introduce the requirement for genome build Bioc packages, which take a while to install. Just something to consider.

If you put these packages in the suggests the install time won't count in the R CMD Check time though? - Like the approach we use in MungeSumstats.

bschilder commented 2 years ago

If you put these packages in the suggests the install time won't count in the R CMD Check time though? - Like the approach we use in MungeSumstats.

True! Sorry, that's actually what I meant. But bc my GHA pipeline installs all dev deps (Imports and Suggests) it just takes a bit longer. Not a big deal, just means I have to wait a bit longer to finish each workflow.

bschilder commented 2 years ago

Removed code inside deprecated functions, and added unit tests to ensure they throw errors with helpful messages when called: test-deprecation.r

Between this and removing unused functions, test coverage is now >54%.

Screenshot 2022-01-24 at 16 40 51

Will continue to bolster this over time.

bschilder commented 1 year ago

Coverage now boosted to >76% after fixes and extra unit tests. Screenshot 2023-03-02 at 19 33 43

Full coverage report here: https://app.codecov.io/gh/neurogenomics/MAGMA_Celltyping/tree/master/R

NathanSkene commented 1 year ago

Great job, thank you for doing this!

Sent from Outlook for iOShttps://aka.ms/o0ukef

From: Brian M. Schilder @.> Sent: Thursday, March 2, 2023 7:34:54 PM To: neurogenomics/MAGMA_Celltyping @.> Cc: Skene, Nathan G @.>; Mention @.> Subject: Re: [neurogenomics/MAGMA_Celltyping] Increase test coverage (Issue #95)

This email from @.*** originates from outside Imperial. Do not click on links and attachments unless you recognise the sender. If you trust the sender, add them to your safe senders listhttps://spam.ic.ac.uk/SpamConsole/Senders.aspx to disable email stamping for this address.

Coverage now boosted to >76% after fixes and extra unit tests. [Screenshot 2023-03-02 at 19 33 43]https://user-images.githubusercontent.com/34280215/222533443-9a1793fa-e090-49d1-8731-0947bf8398f1.png

Full coverage report here: https://app.codecov.io/gh/neurogenomics/MAGMA_Celltyping/tree/master/R

— Reply to this email directly, view it on GitHubhttps://github.com/neurogenomics/MAGMA_Celltyping/issues/95#issuecomment-1452443906, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AH5ZPEYZZ2QD22ZVFHITU3DW2DY55ANCNFSM5LO6ZUWA. You are receiving this because you were mentioned.Message ID: @.***>