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

Issue 3: Not using DT. #4

Merged
merged 1 commit into from
Apr 4, 2024
Merged

Issue 3: Not using DT. #4

merged 1 commit into from
Apr 4, 2024

Conversation

parksw3
Copy link
Collaborator

@parksw3 parksw3 commented Apr 3, 2024

Description

This PR closes #3 .

  • I got rid of all instances of DT and replaced them with standard data.table uses. I tested that the vignette gives sensible answers.
  • Also updated the vignette file to remove the use of DT.

Checklist

  • My PR is based on a package issue and I have explicitly linked it.
  • I have included the target issue or issues in the PR title in the for Issue(s) issue-numbers: PR title
  • I have read the contribution guidelines.
  • I have tested my changes locally.
  • I have added or updated unit tests where necessary.
  • I have updated the documentation if required.
  • My code follows the established coding standards.
  • I have added a news item linked to this PR.
  • I have reviewed CI checks for this PR and addressed them as far as I am able.

@parksw3
Copy link
Collaborator Author

parksw3 commented Apr 3, 2024

By the way, I don't have permission to commit directly to this repo... so I can't add a reviewer either or make a branch in the main repo... @seabbs

@parksw3 parksw3 changed the title Not using DT. #3: Not using DT. Apr 3, 2024
@parksw3
Copy link
Collaborator Author

parksw3 commented Apr 3, 2024

It looks like this is failing because we have no tests set up...?

@parksw3 parksw3 changed the title #3: Not using DT. Issue 3: Not using DT. Apr 3, 2024
@seabbs
Copy link
Contributor

seabbs commented Apr 4, 2024

By the way, I don't have permission to commit directly to this repo... so I can't add a reviewer either or make a branch in the main repo.

Very annoying. I added you as an admin

@@ -16,12 +15,10 @@ sample_model <- function(model, data, scenario = data.table::data.table(id = 1),
fit <- safe_fit_model(model, data, ...)

if (!is.null(fit$error)) {
out <- out |>
DT(, error := list(fit$error[[1]]))
out[, error := list(fit$error[[1]])]
Copy link
Contributor

Choose a reason for hiding this comment

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

an annoying side effect of linting. Need to add error as a global var in utils to get rid of this flag.

Copy link
Contributor

Choose a reason for hiding this comment

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

or we can update the linter to ignore global var checks (but then Rmd check will still flag this anyway)

Copy link
Contributor

Choose a reason for hiding this comment

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

alternatively the env features we use elsewhere may help here?

Copy link
Contributor

@seabbs seabbs left a comment

Choose a reason for hiding this comment

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

Nice ❤️! The linting flags are from global variables throwing problems and can be dealt with in another issue I think.

We might also want an issue for making some of the data.table a bit cleaner without pipes as rather than doing each operation on a single line we might now want to start doing dt[, :=(one_thing, two_thing, three_thing)].

Looking at the check now to see what the problem is.

@@ -16,12 +15,10 @@ sample_model <- function(model, data, scenario = data.table::data.table(id = 1),
fit <- safe_fit_model(model, data, ...)

if (!is.null(fit$error)) {
out <- out |>
DT(, error := list(fit$error[[1]]))
out[, error := list(fit$error[[1]])]
Copy link
Contributor

Choose a reason for hiding this comment

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

or we can update the linter to ignore global var checks (but then Rmd check will still flag this anyway)

data.table::DT(,
sd := exp(meanlog + (1 / 2) * sdlog ^ 2) * sqrt(exp(sdlog ^ 2) - 1)
)
nat_dt <- data.table::copy(dt)
Copy link
Contributor

Choose a reason for hiding this comment

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

in the original (i.e. above) we aren't making a copy but I think its fine either way (though add might imply no copy?)

draws <- data.table::copy(draws)
draws[, mean := mean - runif(.N, min = 0, max = 1)]
draws[, meanlog := log(mean^2 / sqrt(sd^2 + mean^2))]
draw[, sdlog := sqrt(log(1 + (sd^2 / mean^2)))]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
draw[, sdlog := sqrt(log(1 + (sd^2 / mean^2)))]
draws[, sdlog := sqrt(log(1 + (sd^2 / mean^2)))]

@@ -16,12 +15,10 @@ sample_model <- function(model, data, scenario = data.table::data.table(id = 1),
fit <- safe_fit_model(model, data, ...)

if (!is.null(fit$error)) {
out <- out |>
DT(, error := list(fit$error[[1]]))
out[, error := list(fit$error[[1]])]
Copy link
Contributor

Choose a reason for hiding this comment

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

alternatively the env features we use elsewhere may help here?

@seabbs
Copy link
Contributor

seabbs commented Apr 4, 2024

It looks like this is failing because we have no tests set up...?

Yes exactly. I have only partially added all the package infra this needs unfortunately.

@seabbs seabbs merged commit 1a625cd into epinowcast:main Apr 4, 2024
0 of 2 checks passed
seabbs pushed a commit that referenced this pull request Jan 10, 2025
Former-commit-id: 26eb0ceec6e44cce1a0a8243482f174e64afcb24 [formerly cfa856cceeb0f22d605a3130e90096d9853ed6b3]
Former-commit-id: b27ad83c48ed189f3829cac561cd29b4f96dfeda
seabbs pushed a commit that referenced this pull request Jan 21, 2025
Former-commit-id: 1a625cd
Former-commit-id: 68d2d0232db3dc09efaa429d5641b32b4aee1f2f
seabbs pushed a commit that referenced this pull request Jan 21, 2025
Former-commit-id: 1a625cd
Former-commit-id: 68d2d0232db3dc09efaa429d5641b32b4aee1f2f
seabbs pushed a commit that referenced this pull request Jan 21, 2025
Former-commit-id: 1a625cd
Former-commit-id: 68d2d0232db3dc09efaa429d5641b32b4aee1f2f
seabbs pushed a commit that referenced this pull request Jan 21, 2025
Former-commit-id: 26eb0ceec6e44cce1a0a8243482f174e64afcb24 [formerly cfa856cceeb0f22d605a3130e90096d9853ed6b3]
Former-commit-id: b27ad83c48ed189f3829cac561cd29b4f96dfeda
seabbs pushed a commit that referenced this pull request Jan 21, 2025
Former-commit-id: 26eb0ceec6e44cce1a0a8243482f174e64afcb24 [formerly cfa856cceeb0f22d605a3130e90096d9853ed6b3]
Former-commit-id: b27ad83c48ed189f3829cac561cd29b4f96dfeda
Former-commit-id: 5859e28
seabbs pushed a commit that referenced this pull request Jan 21, 2025
Former-commit-id: 1a625cd
Former-commit-id: 68d2d0232db3dc09efaa429d5641b32b4aee1f2f
Former-commit-id: 90e2ecee4d339f86d175198feb861fb66ffd45b9 [formerly adc6d5c]
Former-commit-id: d59cd10ebb10a929b47a4b9244632fa737df99f8
seabbs pushed a commit that referenced this pull request Jan 21, 2025
Former-commit-id: 1a625cd
Former-commit-id: 68d2d0232db3dc09efaa429d5641b32b4aee1f2f
Former-commit-id: 90e2ecee4d339f86d175198feb861fb66ffd45b9 [formerly adc6d5c]
Former-commit-id: d59cd10ebb10a929b47a4b9244632fa737df99f8
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.

DT not exported from data.table
2 participants