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

More robust message wording test #862

Merged
merged 7 commits into from
Jul 20, 2024

Conversation

MichaelChirico
Copy link
Contributor

@MichaelChirico MichaelChirico commented Jul 16, 2024

Part of Rdatatable/data.table#6284.

The message upstream has been improved (here):

'fun.aggregate' is NULL, but found duplicate row/column combinations, so defaulting to length(). That is, the variables [date, model, observed, interval_range, boundary] used in 'formula' do not uniquely identify rows in the input 'data'. In such cases, 'fun.aggregate' is used to derive a single representative value for each combination in the output data.table, for example by summing or averaging (fun.aggregate=sum or fun.aggregate=mean, respectively). Check the resulting table for values larger than 1 to see which combinations were not unique. See ?dcast.data.table for more details.

This will be released to CRAN soon. In general, it is a bit fragile to rely on the exact wording of another package's errors/warnings/messages. See here for how we try and be a bit more flexible in {data.table} when relying on {base} message text:

https://github.com/Rdatatable/data.table/blob/a01f00f7438daf4612280d6886e6929fa8c8f76e/inst/tests/tests.Rraw#L160-L202

Eventually, we'd like to offer you custom condition classes you can more reliably depend on instead:

Rdatatable/data.table#5913

In fact we already do so for the particular message you're relying on: dt_missing_fun_aggregate_warning:

https://github.com/Rdatatable/data.table/blob/0030b15d1ebc9242e30159351388ef3cc114e344/R/fcast.R#L185

@seabbs seabbs requested a review from nikosbosse July 17, 2024 11:32
@nikosbosse
Copy link
Contributor

@MichaelChirico thanks a lot for your recent contributions - much appreciate it. If you like, please add yourself as a contributor to the package DESCRIPTION.

@MichaelChirico
Copy link
Contributor Author

If you like, please add yourself as a contributor to the package DESCRIPTION.

Don't feel I've contributed all that much, but thank you for the offer! And thanks for using data.table 👍

Copy link
Contributor

@nikosbosse nikosbosse left a comment

Choose a reason for hiding this comment

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

This looks great, thanks a lot!

@nikosbosse nikosbosse enabled auto-merge July 19, 2024 09:56
@nikosbosse nikosbosse merged commit 396bd4d into epiforecasts:main Jul 20, 2024
9 checks passed
@MichaelChirico MichaelChirico deleted the patch-1 branch July 30, 2024 04:49
@MichaelChirico
Copy link
Contributor Author

FYI data.table will release to CRAN in August, so expect to need a {scoringutils} update in the near future. Thanks again!

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