worldbank / povcalnetR

R client to the Povcalnet API
https://worldbank.github.io/povcalnetR
Other
9 stars 5 forks source link

Popshare instead of quantile_poor #28

Closed randrescastaneda closed 4 years ago

randrescastaneda commented 4 years ago

Hi Tony,

From your branch “quantile_qp” I checked out another branch “popshare” and implemented all the changes discussed by email. Now, povcalnetR can query the share of the population to get the corresponding poverty by using the following coding syntax,

povcalnet(popshare = .5)
povcalnet_wb(popshare = .5)

I regenerated the documentation and examples. No error was found in the checking process, but I got few warnings. However, they seem to be trivial. Whenever you have a chance could you please check? I also created a pull request if you think it is ready to be merged into master.

Best,

tonyfujs commented 4 years ago

Hey @randrescastaneda I made a few changes to remove code redundancies. Now that they are no longer needed, I basically removed the high-level povcalnet_qp() and povcalnet_wb_qp() functions, and implemented the conditionality at the build_query_string() level instead.

One minor thing I would like to do before merging is bring back the default 1.9 value for povline as a default function argument (as this is the standard way of implementing default values in R, and is inconsistent with how with set the default value for "country"). Let me know what you think. Happy to change my mind if you think there is a good argument not to do it this way.

randrescastaneda commented 4 years ago

Hi @tonyfujs,

Thank you for working on the this feature. I think it is a great idea to implement the *_qp() functions conditionally.

Regarding your question on the default povline my reasoning is the following. Arguments povline and popshare are mutually exclusive. So, if both argument are defined (i.e., in this case, both are different to NULL), we have two options.

  1. one of them overrules the other (e.g., the value if popshare is used and not the one in povline)
  2. there is an error.

If we go for option (1), the only solution is for popshare to prevail over povline because, if by default povline is 1.9, povline is always defined (i.e., never NULL). So, if the user defines popshare the function should return the output of popshare even though povline is defined as 1.9 by default.

Thus, the condition that accounts for the cases where both arguments are defined would have to change from this,

 # If povline and popshare are determined
  if(!is.null(povline) & !is.null(popshare)) {
    stop("You must select either `povline` or `popshare` but no both")
  }

To this,

 # If povline and popshare are determined
  if(!is.null(povline) & !is.null(popshare)) {
    povline <- NULL
   message("value of `povline` is irrelevant when `popshare is defined")
  }

Now, you may say, "this condition is not necessary because we are executing a particular code when popshare is defined and a different code when popshare is not defined, thus the value of povline is irrelevant." And you're right, but the user must be at least notified that the value of povline is not used when popshare is defined.

On the other hand, we could go for option (2) as I suggest. The user will know that povline and popshare and mutually exclusive, but if none of them is defined by the user, the code will use by default povline = 1.9, even though it is not properly defined in the default values of the function's arguments.

In any case, I think both approaches achieve the objective.

What do you think?

Best,

tonyfujs commented 4 years ago

Thanks @randrescastaneda

Very good point. I had not actually thought about the implications of setting the 1.9 default value as a default function argument. Let's go ahead with your implementation then.