epiverse-trace / simulist

An R package for simulating line lists
https://epiverse-trace.github.io/simulist/
Other
5 stars 1 forks source link

set risks to `NA` if processes not desired #146

Closed sbfnk closed 3 months ago

sbfnk commented 4 months ago

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 commented 4 months ago

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

joshwlambert commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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.