Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix toolAggregate edge case #227

Merged
merged 7 commits into from
Nov 11, 2024
Merged

Conversation

pascal-sauer
Copy link
Contributor

toolAggregate would fail if it should do a simple renaming (rel is a 1 to 1 mapping) where a weight with output categories (as opposed to input categories) is passed, see test added in this PR. This PR fixes that.

mapping <- data.frame(a = c("A2", "B1"), b = c("C", "D"))
weight <- pm
getItems(weight, 3) <- c("C", "D")
expect_silent(toolAggregate(pm, mapping, from = "a", to = "b", dim = 3, weight = weight))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this PR the following fails (as it should, because from and to are swapped here):

  toolAggregate(pm, mapping, from = "b", to = "a", dim = 3, weight = weight)

With this PR this no longer fails, instead 'from' and 'to' are simply swapped. However, there are other cases where 'from' and 'to' are swapped opportunistically already, the following is TRUE even without this PR (using pm and map from test-toolAggregate.R):

  a <- toolAggregate(pm, map, from = "reg", to = "from")
  b <- toolAggregate(pm, map, from = "from", to = "reg")
  identical(noC(a),noC(b))

While I don't like this at all, it seems be okay as this problem is already present and also somewhat inherent to the ultra flexible interface and "smart" default values if from and to are not set explicitly. I've spent quite a while on finding a solution without this downside, but couldn't find one. toolAggregate is doing so many things, allowing input in so many formats and combinations that I find it has become a serious problem to further work on/fix bugs in toolAggregate.

@pascal-sauer pascal-sauer merged commit 07a5077 into pik-piam:master Nov 11, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants