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

Bug in filter_obs_by_obs_time #359

Closed
athowes opened this issue Oct 1, 2024 · 2 comments · Fixed by #360
Closed

Bug in filter_obs_by_obs_time #359

athowes opened this issue Oct 1, 2024 · 2 comments · Fixed by #360
Assignees
Labels
bug Something isn't working

Comments

@athowes
Copy link
Collaborator

athowes commented Oct 1, 2024

In PR #349 I failed to rename properly in filter_obs_by_obs_time.

It should have been:

filter_obs_by_obs_time <- function(linelist, obs_time) {
  linelist |>
    mutate(
      relative_obs_time = obs_time - .data$ptime,
      censored_obs_time = .data$obs_time - .data$ptime_lwr,
      censored = "interval"
    ) |>
    filter(.data$stime_upr <= .data$obs_time)
}

But I had:

filter_obs_by_obs_time <- function(linelist, obs_time) {
  linelist |>
    mutate(
      obs_time = obs_time - .data$ptime,
      censored_obs_time = .data$obs_time - .data$ptime_lwr,
      censored = "interval"
    ) |>
    filter(.data$stime_upr <= .data$obs_time)
}
@athowes athowes added the bug Something isn't working label Oct 1, 2024
@athowes athowes self-assigned this Oct 1, 2024
@athowes
Copy link
Collaborator Author

athowes commented Oct 3, 2024

So here:

  1. censored_obs_time is not used anywhere in the code and I think we can get rid
  2. censored = "interval" is also not used anywhere in the code and I think we can get rid

@athowes
Copy link
Collaborator Author

athowes commented Oct 3, 2024

Also to note I think filter_obs_by_ptime is 1. not used anywhere in the code 2. likely buggy (it's untested like filter_obs_by_obs_time).

In #34 I proposed to merge these two functions but didn't get around to it. Perhaps look to do that?

Perhaps easy to write again from scratch rather than understand existing?

Edit: I think also as part of this issue should add testing for some "observe" functionality.

seabbs pushed a commit that referenced this issue Oct 4, 2024
* Fix to filter_obs_by_obs_time function

* Strange bug fix: reordering the columns makes a difference. Needs issue to fix

* Remove "censored_obs_time" and "censored" columns and include "obs_time" as a column

* Lint code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant