Skip to content

Clean up a few more issues #85

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

Merged
merged 33 commits into from
Jul 5, 2022
Merged

Clean up a few more issues #85

merged 33 commits into from
Jul 5, 2022

Conversation

admin and others added 30 commits May 10, 2022 16:53
Merge branch 'main' into epi_slide-error_message

# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
Merge branch 'main' into epi_slide-error_message
Merge branch 'main' into epi_slide-error_message
…tatement

Merge branch 'main' into epi-archive-print
dajmcdon added 3 commits May 25, 2022 12:31
Added final newline to epi_archive print statement.
epi_slide `f` parameter documentation
@dajmcdon dajmcdon requested a review from brookslogan May 25, 2022 21:13
@dajmcdon
Copy link
Contributor Author

dajmcdon commented May 25, 2022

Note: there's some weird formatting. I'm going to clean that up after (to avoid continued maintenance on 2 remotes).

@brookslogan
Copy link
Contributor

  • Confirming that the improved error messaging works to resolve Improve epi_slide error messaging on bad ref_time_values #65. But beyond Improve epi_slide error messaging on bad ref_time_values #65 we should probably be more stringent on ref_time_values and require them to all be in unique(x$time_value) rather than silently filtering to such values + disallow passing in 0 ref_time_values. That should also give users an easier time debugging as they don't have to track down what starts and stops are.
    • ---> This can be filed in a new Issue & addressed later.
  • Re. Improve documentation for epi_slide's f parameter when it's a function with named arguments #66:
    • The main @param f documentation is still wrong and doesn't reference the clarifying @details in this section: "If a function, f must take x, a data frame with the same column names as the original object; followed by any number of named arguments; and ending with ...". I think the proper thing may be that f should take x, g/k/key/think-of-a-better-name, and named args (dots don't seem necessary; I think originally they were just there to absorb the unlisted g).
    • In @details, the following text too specific to the particular example that follows: "if a tibble with an unnamed grouping variable is passed in as the method argument to f". This explanatory text may be unnecessary anyway once the main @param f documentation is fixed. The dots in the example may be unnecessary. The example code could also use a library(dplyr), and these interspersed details&code could be turned into @examples comments&code so users can more easily run them and so package checks ensure that these examples run. (Separately, as part of Implement testing across the package #42, maybe these examples should be turned into tests as well?)
  • Re. Add final newline to epi_archive print statement #68: archive_cases_dv_subset needs to be regenerated or reworked to form an epi_archive from more basic parts on demand; it currently appears to load with the old class definition, missing the extra "\n", plus some bad srcrefs that may make user debugging of tweaked examples harder:
archive_cases_dv_subset$print
<srcref: file "C:/Users/chloe/Desktop/Summer2022/epiprocess/R/archive.R" chars 162:19 to 187:11>
<environment: 0x55c5b02d6358>
Warning message:
In getSrcLines(srcfile, x[7L], x[8L]) :
  restarting interrupted promise evaluation
> body(archive_cases_dv_subset$print)
{
    cat("An `epi_archive` object, with metadata:\n")
    cat(sprintf("* %-9s = %s\n", "geo_type", self$geo_type))
    cat(sprintf("* %-9s = %s\n", "time_type", self$time_type))
    if (!is.null(self$additional_metadata)) {
        sapply(self$additional_metadata, function(m) {
            cat(sprintf("* %-9s = %s\n", names(m), m))
        })
    }
    cat("----------\n")
    cat(sprintf("* %-14s = %s\n", "min time value", min(self$DT$time_value)))
    cat(sprintf("* %-14s = %s\n", "max time value", max(self$DT$time_value)))
    cat(sprintf("* %-14s = %s\n", "min version", min(self$DT$version)))
    cat(sprintf("* %-14s = %s\n", "max version", max(self$DT$version)))
    cat("----------\n")
    cat(sprintf("Data archive (stored in DT field): %i x %i\n", 
        nrow(self$DT), ncol(self$DT)))
    cat("----------\n")
    cat(sprintf("Public methods: %s", paste(names(epi_archive$public_methods), 
        collapse = ", ")))
}

The quick fix is to just regenerate the object + file an Issue to later figure out how to generate the example epi_archive on demand or otherwise ensure that it is current with the implementation of epi_archive. The less-quick fix is to try to immediately address this issue.

@brookslogan
Copy link
Contributor

Re. #68 and regenerating / syncing the example archive object: the simple way is just to re-run to regenerate the archive object. The more complex way is to store its DT and reconstruct the archive on demand; I've implemented this as part of 8f36d44, but based on an older version/name of the archive object, and involving the compactify parameter under development in #101, so it will need a little tweaking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants