Add functionality to run from_actors mode with more than one TF #185
Conversation
Conflicts: src/confounders.jl test/confounders.jl
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #185 +/- ##
==========================================
- Coverage 95.72% 95.64% -0.09%
==========================================
Files 5 5
Lines 351 367 +16
==========================================
+ Hits 336 351 +15
- Misses 15 16 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thank you for taking over this Breeshey, I've started the review. I think there is one major point and one less important but still nice to address before I move on with checking correctness and the test. I've also added some comments regarding code style, some superfluous functions and some changes that I think need to be undone.
- Unless I am missing something, the change is breaking at the user level because now the TF column is mandatory. I think it would be best to keep it optional because users only looking at a single transcription factor will not necessarily find useful to define this column.
- I think the change in the name of the ouput would be more easily read if the outprefix was changed instead.
In any case, because of the output name change this PR will be breaking at the pipeline's level and may require some changes in the targene-pipeline itself (this is entirely fine!).
There was a problem hiding this comment.
Thank you Breeshey, this looks good to me! Good usage of multiple dispatch in the filter_snps_by_tf function. The missed covered line might be because they are smart enough to realise that the condition is never met in the tests. I've added this issue for future reference. But nothing to be concerned about for now! Feel free to merge, thanks!
When running interactively, all TargeneCore tests pass with no errors.