reconhub / incidence

☣:chart_with_upwards_trend::chart_with_downwards_trend:☣ Compute and visualise incidence
https://reconhub.github.io/incidence
Other
58 stars 13 forks source link

rename option `iso_weeks` to `iso` #55

Closed zkamvar closed 6 years ago

zkamvar commented 6 years ago

Because we now support Month, Quarter, and Year intervals, it would make sense to make a clear option for the user to be able to set the ISO definitions, specifying that the period should begin at the beginning of the month/quarter/year

caijun commented 6 years ago

I don't understand "specifying that the period should begin at the beginning of the month/quarter/year", every calendar date have a corresponding ISO week date formate, from which we can extract the week information, then we can aggregate the events into weekly events without specifying the starting date.

zkamvar commented 6 years ago

This is in part because of the features added due to #26, The ISO week date does not correspond well with monthly intervals, but users may want to specify that the intervals begin on the first of the month.

if we switch iso_week to iso, it would retain the behavior of iso_week when the user specifies an interval of 7 or "week", but also allows us to give the user a choice of specifying an alternative start date with a "month", "quarter" or "year" interval by setting iso = FALSE

caijun commented 6 years ago

I see. Since now the interval parameter can take characters, how about using week and ISOweek separately represent the week/7 day interval and the ISO week format? This will reduce the number of parameters. And in the future, we can support more character intervals, such as other week formats. I planned to create an issue to discuss this. Now we can discuss this under this issue.

thibautjombart commented 6 years ago

I like having iso = TRUE by default so that when interval is week, month, or quarter, we use ISO by default; that still saves characters ;)

caijun commented 6 years ago

To makeincidence package widely useful, the incidence package should support various epidemiological week definitions (at least 6 types of weeks, see https://github.com/reconhub/incidence/issues/24), such as the week/7 day interval with specifying the starting date, the ISO week (first day of ISO week is Monday), the MMWR week(first day of MMWR week is Sunday), etc. I give such a suggestion because ISOweek is just one of various weeks used in epidemiolgical reports. If we add an extra parameter for ISOweek, then how about MMWR week?

thibautjombart commented 6 years ago

I think the ISO would be a reasonable default for weeks, months, quarters, years, and then one could easily implement extra week definitions e.g. interval = "MMWRweek" ?

caijun commented 6 years ago

I agree. The ISOweek is used in WHO epidemiological reports.

interval = 7L represents the week/7 day interval with specifying the starting date.

interval = "ISOweek" represents the ISO week, which is the default week definition.

interval = "MMWRweek" represents the MMWR week.

interval = "other week names" represents the week definitions that will be supported/implemented in the future.

thibautjombart commented 6 years ago

Yeah. And the advantage is that this is merely incremental changes, so easy for new additions to be made / PRed.