talgalili / dendextend

Extending R's Dendrogram Functionality
153 stars 29 forks source link

Added Tests for Untangle.R #128

Closed alecbuetow closed 1 month ago

alecbuetow commented 1 month ago

I tried to achieve full coverage but I don't think it will be possible for all_couple_rotations_at_k lines 523, 528-534. I believe neither k_cluster_leaves nor km1_cluster_leaves can be NA, since both variables depend on the function cutree() which has the default parameter NA_to_0L = TRUE, setting all NA values to 0 instead.

I also had to change untangle_intercourse to create a working test for untangle_evolution. I think the corrections make the function more logically consistent; as it currently stands, untangle_intercourse takes four dendrograms as its input, then untangles #1 and #4 together, then #2 and #3 together. When it returns your newly untangled dendrograms, they are in the order: 1, 4, 3, 2. The other comparable functions to this one all untangle #1 and #2 together, then #3 and #4 together, returning them in the order: 1, 2, 3, 4. My changes made untangle_intercourse do the same.

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 41.77%. Comparing base (f3a27a6) to head (596a3b4). Report is 1 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #128 +/- ## ========================================== + Coverage 39.83% 41.77% +1.93% ========================================== Files 50 50 Lines 4182 4182 ========================================== + Hits 1666 1747 +81 + Misses 2516 2435 -81 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

talgalili commented 1 month ago

All sounds good to me. I'm not sure you should have changed the dendlist to list (there are other ways to have fixed it for testing). But I approved all changes for now.

Thanks for the work! I'd love to see more diffs :)

alecbuetow commented 1 month ago

Amazing, I'm happy to go back and modify the function and tests if you'd like to use a dendlist. Won't be hard to modify it now that everything else is already set

talgalili commented 1 month ago

Thanks, yes, please do.

On Wed, 30 Oct 2024, 8:51 Alec Buetow, @.***> wrote:

Amazing, I'm happy to go back and modify the function and tests if you'd like to use a dendlist. Won't be hard to modify it now that everything else is already set

— Reply to this email directly, view it on GitHub https://github.com/talgalili/dendextend/pull/128#issuecomment-2447042801, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHOJBXEIX55I2WZ7L443N3Z6DI4DAVCNFSM6AAAAABQ2YJPAWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINBXGA2DEOBQGE . You are receiving this because you modified the open/close state.Message ID: @.***>