MarioniLab / miloDE

Framework for sensitive DE testing (using neighbourhoods)
Other
57 stars 2 forks source link

Code review #19

Closed MikeDMorgan closed 1 year ago

MikeDMorgan commented 1 year ago

Be explicit about function imports - setdiff isn't defined anywhere (also, why not just use the base::diff?): https://github.com/MarioniLab/miloDE/blob/d763aa0fa6c11cbf17eb08c108c6dfe18b51b168/R/de_test_all_hoods.R#L59

Don't use F and T as these can easily be overwritten by users, use TRUE and FALSE explicitly: https://github.com/MarioniLab/miloDE/blob/d763aa0fa6c11cbf17eb08c108c6dfe18b51b168/R/de_test_all_hoods.R#L63 https://github.com/MarioniLab/miloDE/blob/d763aa0fa6c11cbf17eb08c108c6dfe18b51b168/R/de_test_all_hoods.R#L99 https://github.com/MarioniLab/miloDE/blob/d763aa0fa6c11cbf17eb08c108c6dfe18b51b168/R/de_test_all_hoods.R#L106

You can define the output order of topTags, it doesn't need to be done here, or set topTags(..., sort.by='none'): https://www.rdocumentation.org/packages/edgeR/versions/3.14.0/topics/topTags https://github.com/MarioniLab/miloDE/blob/d763aa0fa6c11cbf17eb08c108c6dfe18b51b168/R/de_test_all_hoods.R#L254

amissarova commented 1 year ago

@MikeDMorgan - setdiff is from the base package - what do you mean by 'setdiff isn't defined anywhere'?

MikeDMorgan commented 1 year ago

My bad - ignore that

amissarova commented 1 year ago

done