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

Iterate on compactify_tol interface & usage #570

Open
brookslogan opened this issue Nov 19, 2024 · 1 comment · May be fixed by #601
Open

Iterate on compactify_tol interface & usage #570

brookslogan opened this issue Nov 19, 2024 · 1 comment · May be fixed by #601
Assignees

Comments

@brookslogan
Copy link
Contributor

brookslogan commented Nov 19, 2024

Currently, it's hard to set compactify_tol since we have to go through the less-convenient new_epi_archive. We should provide compactify_tol as an arg of as_epi_archive.

The default compactify_tol may not be appropriate if the user has per-capita rate data (rather than e.g. per-100k); it may be safer to use 0 --- and make 0 actually work; dplyr::near checks < tol and we'd need <= tol. This also prevents surprises when performing epix_slides that should preserve compactness and converting to epi_archive still talking about compactifying.

Really, compactify and compactify_tol shouldn't even be in new_epi_archive to begin with according to tidy design or some other prominent guidelines, since new_* are supposed to do just simple class validation checks.

@brookslogan brookslogan changed the title Make compactify_tol configurable from as_epi_archive, consider 0 as default Iterate on compactify_tol interface & usage Jan 16, 2025
@brookslogan
Copy link
Contributor Author

brookslogan commented Jan 16, 2025

Also, we probably shouldn't be using tol when looking at the epikeytime columns. E.g., if we are using daily counts and decide a difference of a few counts can be neglected, we don't want a tol, say, in the 1--3 range to cause us to potentially skip time_values altogether in flat periods.

@brookslogan brookslogan self-assigned this Jan 28, 2025
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 a pull request may close this issue.

1 participant