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

Use reframe() instead of summarize() for multiple rows #620

Merged
merged 104 commits into from
May 12, 2023
Merged

Conversation

capnrefsmmat
Copy link
Contributor

Since dplyr 1.1.0, returning multiple rows from summarize() is deprecated, and you're supposed to use reframe() instead. These are our two use-cases, so switch them over.

I ran into this while updating an old presentation, and dplyr helpfully barfed the deprecation warning right into my slides. This should fix it.

nmdefries and others added 30 commits July 8, 2021 15:01
Fix `problem_dates` check when fetching actual data from API
…roup

Remove duplicate join in `evaluate_covid_predictions`
…val-preds-group

Revert "Remove duplicate join in `evaluate_covid_predictions`"
Get unit tests to pass on `evalcast` branch
Retry forecaster file download if prior attempts fail
…neral

Forecast eval support: make retries more general
* add honest_as_of flag
* add parallel_execute flag
Retry truth data request from COVIDcast if prior attempts fail
@dajmcdon
Copy link
Collaborator

dajmcdon commented May 6, 2023

My memory is that the reason it's not on main was to avoid forcing @capnrefsmmat and @krivard to be required for PR review. I think at some point, there was concern that anything on main in this repo was "public" and therefore needed more stringent approvals. But some packages had (at the time) much faster dev cycles.

Longer term, I would suggest that any packages that are still useful to be moved out of subfolders in this repo into their own repos. And perhaps kill off stale junk.

@dajmcdon
Copy link
Collaborator

dajmcdon commented May 6, 2023

The failed evalcast test is potentially concerning. But @nmdefries is more up to speed on that package's status.

@capnrefsmmat
Copy link
Contributor Author

My memory is that the reason it's not on main was to avoid forcing @capnrefsmmat and @krivard to be required for PR review. I think at some point, there was concern that anything on main in this repo was "public" and therefore needed more stringent approvals. But some packages had (at the time) much faster dev cycles.

Before covidcast was on CRAN, we were certainly worried that most people installed from GitHub, so changes would immediately be visible to users. Does evalcast get used by outsiders in that way, or is everyone using evalcast also involved in development?

We can also update the CODEOWNERS file if it's constantly requiring unnecessary reviews. It's probably worth reviewing it anyway, as several people have left.

@dshemetov
Copy link
Collaborator

Pinging @lcbrooks for evalcast usage input. I have a hunch that we're the only ones using it, but maybe not.

The current evalcast CI failure is coming from bettermc being booted off CRAN. @nmdefries made a fix already here, I'm going to apply a similar fix on the evalcast branch and merge it here.

@brookslogan
Copy link
Collaborator

brookslogan commented May 9, 2023

I'm not aware of other people who are using evalcast, and I can't find any usage on GitHub (searches A, B give only false positive plus hadley's covidcast fork (probably from updating covidcast package's tidyselect usage due to CRAN checks)) or on the web. So I think we're free to do whatever we want here regarding evalcast branch management.

[Guess I should have also checked for DESCRIPTION files / etc. to see if there are packages that depend on evalcast. But the main one to check is covidHubUtils but it also doesn't use evalcast, so I'm guessing there aren't.]

Copy link
Contributor

@krivard krivard left a comment

Choose a reason for hiding this comment

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

CODEOWNERS and CI workflow look solid; minor question on working directory settings

.github/workflows/r_evalcast_ci.yml Show resolved Hide resolved
Copy link
Contributor

@krivard krivard left a comment

Choose a reason for hiding this comment

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

👍 ci changes approved

@@ -94,4 +94,4 @@ Suggests:
VignetteBuilder:
utils
Depends:
R (>= 3.5.0)
R (>= 4.1.0)
Copy link
Contributor

Choose a reason for hiding this comment

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

for a hot second I was extremely confused, because the CI changes from this commit block are for covidcast, and this version dependency change is for evalcast

but i've confirmed that the only R version in the evalcast ci is 4.1, so this change should work fine with ci

@dshemetov dshemetov merged commit 0b7b523 into main May 12, 2023
@dshemetov dshemetov deleted the dplyr-summarize branch May 12, 2023 18:26
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.

8 participants