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

set risks to NA if processes not desired #146

Closed
sbfnk opened this issue Jun 20, 2024 · 8 comments
Closed

set risks to NA if processes not desired #146

sbfnk opened this issue Jun 20, 2024 · 8 comments
Milestone

Comments

@sbfnk
Copy link
Contributor

sbfnk commented Jun 20, 2024

Setting onset_to_hosp to NA yields a warning

Warning messages:
1: Some onset-to-event and their corresponding risk are incompatible:
  - onset_to_hosp is set to NA but hosp_risk is specified 

I think it should be either documented with the onset_to_hosp argument that this has to be done manually, or (my preference) hosp_risk should be set automatically if it is not specified (can use missing()) and onset_to_hosp is not wanted.

@jamesmbaazam
Copy link
Member

I think this and #147 are meant to be posted on {simulist}, so I'll transfer them there.

@jamesmbaazam jamesmbaazam transferred this issue from epiverse-trace/epichains Jun 20, 2024
@joshwlambert
Copy link
Member

The hosp_risk argument is set by default to 0.2. Is your preference to override the hosp_risk to NULL with onset_to_hosp is set to NULL and not warn the user? (NULL rather than NA since PR #147).

I'd prefer not to remove the default value for the *_risk arguments as we are moving in the direction of adding more default values rather than less (see #120).

@sbfnk
Copy link
Contributor Author

sbfnk commented Jun 24, 2024

How about something like

if (is.null(onset_to_hosp) && missing(hosp_risk)) {
  hosp_risk <- NULL
}

and add to the documentation that hosp_risk ignored if onset_to_hosp is null?

@joshwlambert
Copy link
Member

joshwlambert commented Jun 25, 2024

I would like to keep the default values for the *_risk arguments to enable users to easily run the function without having to specify them.

The choice as I see it is between

A) No warning to users

if (is.null(onset_to_hosp)) {
  hosp_risk <- NULL
}

B) Warn users that hosp_risk is being ignored

if (is.null(onset_to_hosp) && !is.null(hosp_risk)) {
  warning(...)
  hosp_risk <- NULL
}

Whichever option is chosen I will improve the documentation.

@sbfnk
Copy link
Contributor Author

sbfnk commented Jul 1, 2024

I would like to keep the default values for the *_risk arguments to enable users to easily run the function without having to specify them.

I'm not completely following - my suggestion would allow you to keep the default but warn if set by a user if they also set onset_to_hosp to NULL.

@joshwlambert
Copy link
Member

joshwlambert commented Jul 1, 2024

This is a good solution, but how do you know if hosp_risk is set by the user when onset_to_hosp is set to NULL?

For example if hosp_risk is not 0.2, then you know it's been changed by the user from the default value, but what if the user sets it to 0.2 and sets onset_to_hosp to NULL, should it still warn?

Phrased another way, is there a way to know if a user has supplied a value to an argument if that argument has a default (which precludes the use of missing())?

This was written without fully understanding the behaviour of missing(), see comment below for correction.

@joshwlambert
Copy link
Member

joshwlambert commented Jul 1, 2024

First and foremost apologies, I didn't understand the functionality of missing() fully. It appears that missing() is TRUE even when the argument has a default value:

func <- function(x = 1) {
  print(x)
  if (missing(x)) {
    return("it is missing")
  } else {
    return("it is not missing")
  }
}
func()
#> [1] 1
#> [1] "it is missing"
func(x = 2)
#> [1] 2
#> [1] "it is not missing"

Created on 2024-07-01 with reprex v2.1.0

I'll implement the changes you suggested using missing().

(Sorry, this wouldn't have taken much less back-and-forth if my knowledge of R was better).

@sbfnk
Copy link
Contributor Author

sbfnk commented Jul 1, 2024

No worries, and clearly not the most obvious behaviour! missing() is a bit magical, only problem being that (I think) there is no way to pass missingness on to lower-level functions.

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

No branches or pull requests

3 participants