Page MenuHomePhabricator
Create Task
Maniphest T28097

test errors in aggregateThenRank and rankThenAggregate
Closed, ResolvedPublic
Actions

Description

everything with "reverse order" is not working for me because ranking list is the same as without reversing ordering, does this work for you @eisenman

test-rankThenAggregate.R:29: failure: rank-then-aggregate by mean works with two algorithms (reverse order) for one case, small values are better
ranking$matlist$T1 not equal to `expectedRanking`.
Attributes: < Component "row.names": 2 string mismatches >
Component "rank_mean": Mean relative difference: 0.6666667
Component "rank": Mean relative difference: 0.6666667

test-rankThenAggregate.R:60: failure: rank-then-aggregate by mean works with two algorithms (reverse order) for one case, large values are better
ranking$matlist$T1 not equal to `expectedRanking`.
Attributes: < Component "row.names": 2 string mismatches >
Component "rank_mean": Mean relative difference: 0.6666667
Component "rank": Mean relative difference: 0.6666667

Related Objects

Event Timeline

wiesenfa created this task.Dec 10 2020, 11:11 AM
eisenman added a subscriber: reinkea.EditedDec 11 2020, 9:45 AM

It works for me on the latest develop branch (#921705).

eisenman added a comment.Dec 11 2020, 9:51 AM
This comment was removed by eisenman.
wiesenfa added a comment.Dec 14 2020, 6:47 PM

seems to not fail on windows machines. but does on my Mac (and thus might also on linux). minor issue, defer to post 1.0.0

eisenman added a comment.Dec 17 2020, 1:50 PM

The platform might not be the (only) reason.
@wiesenfa: Which version of R and testthat are you using?

I have the same errors on Ubuntu 18.04 (R version 4.0.3, testthat 3.0.0).

For testthat 3.0.0 some breaking changes are mentioned here: https://github.com/r-lib/testthat/releases

wiesenfa added a comment.Dec 17 2020, 4:45 PM

R 4.0.2.
testthat previously 2.3.2, now 3.0.1.
the issue is however not connected to test_that I see that in the first 2 tests in aggregate-then-rank the ordering in the data set is not used but sorted

# first test
   data <- rbind(
+     data.frame(algo="A1", value=0.6, case="C1"),
+     data.frame(algo="A2", value=0.8, case="C1"))
>   challenge <- as.challenge(data, taskName="T1", algorithm="algo", case="case", value="value", smallBetter = TRUE)
>   ranking <- challenge%>%aggregateThenRank(FUN = mean)
>   ranking
$T1
   value_mean rank
A1        0.6    1
A2        0.8    2

>   expectedRanking <- rbind(
+     "A1" = data.frame(value_mean = 0.6, rank = 1),
+     "A2" = data.frame(value_mean = 0.8, rank = 2))
>   expect_equal(ranking$matlist$T1, expectedRanking)

# second test

>   data <- rbind(
+             data.frame(algo = "A2", value = 0.8, case = "C1"),
+             data.frame(algo = "A1", value = 0.6, case = "C1"))
>   challenge <- as.challenge(data, taskName="T1", algorithm = "algo", case = "case", value = "value", smallBetter = TRUE)
>   ranking <- challenge%>%aggregateThenRank(FUN = mean)
>   ranking
$T1
   value_mean rank
A1        0.6    1
A2        0.8    2

>   expectedRanking <- rbind("A2" = data.frame(value_mean = 0.8, rank = 2),
+                            "A1" = data.frame(value_mean = 0.6, rank = 1))
>   expect_equal(ranking$matlist$T1, expectedRanking)
Fehler: ranking$matlist$T1 not equal to `expectedRanking`.
Attributes: < Component “row.names”: 2 string mismatches >
Component “value_mean”: Mean relative difference: 0.2857143
Component “rank”: Mean relative difference: 0.6666667
eisenman added a comment.Dec 17 2020, 4:59 PM

Is the order of the rows relevant in any processing step? If not, the tests can be removed. Currently, I don't see a way making it work for both systems I have available for testing (Win10, R 3.6.3 and Ubuntu 18.04, R 4.0.3).

wiesenfa added a comment.Dec 17 2020, 5:13 PM

don't think its necessary. there is anyway ordering applied to it in later stages.
otherwise ranking step (rank.aggregated.list()) could always do the ordering directly (then the test would check whether ordering is working). As mentioned before I avoided such change before the release because of (very unlikely) risk of breaking something in the first level hierarchy

eisenman claimed this task.Dec 17 2020, 6:03 PM
eisenman raised the priority of this task from Normal to High.
eisenman moved this task from Backlog to In Progress on the challengeR (v1.0) board.
eisenman mentioned this in rCHALLENGER1239f2bdc6aa: Merge branch 'feature/T28097-IrrelevantRowOrder' into develop.Dec 17 2020, 6:37 PM
eisenman moved this task from In Progress to Done on the challengeR (v1.0) board.Dec 17 2020, 6:38 PM
eisenman closed this task as Resolved.Dec 20 2020, 9:21 PM
MITK · MIC · IMSY · DKFZ