nutriverse / mwana

http://nutriverse.io/mwana/
GNU General Public License v3.0
2 stars 0 forks source link

Tidy up and neat index organisation on package website #51

Closed tomaszaba closed 1 week ago

tomaszaba commented 1 week ago

@ernestguevarra, here is my suggestion on how we can tidy up mwana's package index:

Package index

On this one for now we have three functions for each plausibility. I am thinking about refactoring this into a single function that user can control with an argument .for = c("wfhz", "mfaz", "muac"). What do you think?

For this, currently we have four functions that estimated wasting prevalence based on wfhz, mfaz, muac and combined. The arguments to pass to each function are the same. Last night I thought about wrapping all this in just one function that can be controlled by the argument .based_on. The pseudocode is something as:

mw_estimate_wasting_prevalence <- function(
.wt = NULL, 
.edema = NULL, 
.summarise_at = NULL,
.based_on = c("wfhz", "muac", "mfaz", "combined")
) {

# Match argument .based_on ----
 ## Code 

# Control flow ----
if(.based_on == "wfhz") {
estimate prevalence based on wfhz
}
if (.based_on == "mfaz") {
estimate prevalence based on mfaz
}
if (.based_on == "muac") {
estimate prevalence based on muac 
}
if(.based_on == "combined") {
estimate prevalence based on combined definitions
}
}

In this way, the exported function for prevalence estimation would just be the above.

What do you think about this and about the suggested package index.

ernestguevarra commented 1 week ago

@tomaszaba this is fantastic! I like it.

Following are minor comments:

  1. looking that the wrangle stuff and because you really like that, it makes more sense to use wrangle with the age function also. I don't like it, but you like it and that is what is important. and I think most people will understand it.

  2. for the functions that you want to collapse into one, I am a bit worried about that but I can see where you are coming from. What I would like to suggest is that the functions used for the one function should also be exported so that users can have a choice of potentially using those primitives rather than the one big function

  3. Related to the one big function for prevalence, I need to see the implementation of this to fully get on board. My main worry is about code maintenance. But I can be persuaded.

Please go ahead with these. They are fantastic!

Please use a new branch from dev for your re-factor process.

tomaszaba commented 1 week ago

@tomaszaba this is fantastic! I like it.

Following are minor comments:

  1. looking that the wrangle stuff and because you really like that, it makes more sense to use wrangle with the age function also. I don't like it, but you like it and that is what is important. and I think most people will understand it.

  2. for the functions that you want to collapse into one, I am a bit worried about that but I can see where you are coming from. What I would like to suggest is that the functions used for the one function should also be exported so that users can have a choice of potentially using those primitives rather than the one big function

  3. Related to the one big function for prevalence, I need to see the implementation of this to fully get on board. My main worry is about code maintenance. But I can be persuaded.

Please go ahead with these. They are fantastic!

Please use a new branch from dev for your re-factor process.

Thanks for the feedback, Ernest. All well noted. I'm confident that I will persuade you on this one very easily 😅.