PIP-Technical-Team / pipapi

What the Package Does (One Line, Title Case)
https://pip-technical-team.github.io/pipapi/
Other
3 stars 0 forks source link

new endpoint for grouped stats #393

Closed shahronak47 closed 6 months ago

shahronak47 commented 11 months ago

Adding a new endpoint /grouped-stats which calls wbpip:::gd_compute_pip_stats . If you have a better name for this endpoint please let me know.

tonyfujs commented 10 months ago

Hi @shahronak47 ,

I could not make it work (see bellow). Can you please provide me with an example of how it works? Also, could you please add a few unit tests?

// http://127.0.0.1:8080/api/v1/grouped-stats?welfare=0.1,0.2,0.3,0.4,0.5,0.6,0.7,0.8,0.9,1&weight=0.1,0.2,0.3,0.4,0.5,0.6,0.7,0.8,0.9,1

{
  "error": "500 - Internal server error",
  "message": "Error in strsplit(params$population, \",\"): non-character argument\n"
}

Thanks.

@randrescastaneda You are getting this error because the you are passing a weight parameter in your query, when the endpoint is expecting a population parameter. There are also other parameters that are required for the underlying function to work: image

A few thoughts about this:

randrescastaneda commented 10 months ago

Hi @shahronak47, I create this substask with instructions for this PR CU-86872qgw9 Basic stats

Also, make sure to follow the directions of this other task, regarding the development of these endpoints.

Thanks.

shahronak47 commented 10 months ago
  • requested_mean should probably be named mean

Where do we want to do this change? In the API or the underlying function wbpip:::gd_compute_pip_stats. Right now, I have kept all the arguments in sync with the function call.

My personal opinion is the arguments to API and the function arguments should be in-sync so we should do this change in the function and then API will follow suit. Let me know what you think @randrescastaneda @tonyfujs

tonyfujs commented 10 months ago
  • requested_mean should probably be named mean

Where do we want to do this change? In the API or the underlying function wbpip:::gd_compute_pip_stats. Right now, I have kept all the arguments in sync with the function call.

My personal opinion is the arguments to API and the function arguments should be in-sync so we should do this change in the function and then API will follow suit. Let me know what you think @randrescastaneda @tonyfujs

Hi @shahronak47. Good point, but in that case I don't think we should rename the function's argument as we have two various types of mean arguments in PIP. Let me check and get back to you.

shahronak47 commented 10 months ago

We should probably not expect end-users to specify their own ppp, so the default_ppp parameter should probably be removed and defined in the anonymous functions that underpins the endpoint.

The underlying function has default value for ppp (default_ppp) as 1, the API takes the default value if nothing is provided.

shahronak47 commented 10 months ago

@randrescastaneda I am using these values to test the API.

Welfare - "0.0002,0.0006,0.0011,0.0021,0.0031,0.0048,0.0066,0.0095,0.0128,0.0177,0.0229,0.0355,0.0513,0.0689,0.0882" (Enter without quotes in Swagger UI) Population - "0.001,0.003,0.005,0.009,0.013,0.019,0.025,0.034,0.044,0.0581,0.0721,0.1041,0.1411,0.1792,0.2182" Enter without quotes in Swagger UI) requested_mean = 2.911786 povline = 1.9

randrescastaneda commented 10 months ago

We should probably not expect end-users to specify their own ppp, so the default_ppp parameter should probably be removed and defined in the anonymous functions that underpins the endpoint.

The underlying function has default value for ppp (default_ppp) as 1, the API takes the default value if nothing is provided.

Hi @shahronak47. Thank you, I understand now the logic of your decision. The bottom line is not to worry about it. You can remove default_ppp from the parameters of this new endpoint or leave the default as 1.

The reason is the following. There are two ppp parameters, ppp and default_ppp. ppp is only relevant when the requested_mean is in Local Currency Unit (i.e., the currency of the currency). If the requested_mean is in PPP values already, both ppp and default_ppp play no role in the calculations. default_ppp is only used when the user wants to "play" with the value of the PPP if it were different from they one in which the requested_mean already is. It works like an adjustment factor of the requested_mean. So, either remove it or leave it with 1 as the default. Leaving it with the default makes more sense, in case we want to give that option to experienced users, but let's not advertize it to much to avoid confusion.

@tonyfujs , what do you think?

randrescastaneda commented 10 months ago

The reason is the following. There are two ppp parameters, ppp and default_ppp. ppp is only relevant when the requested_mean is in Local Currency Unit (i.e., the currency of the currency). If the requested_mean is in PPP values already, both ppp and default_ppp play no role in the calculations. default_ppp is only used when the user wants to "play" with the value of the PPP if it were different from they one in which the requested_mean already is. It works like an adjustment factor of the requested_mean. So, either remove it or leave it with 1 as the default. Leaving it with the default makes more sense, in case we want to give that option to experienced users, but let's not advertize it to much to avoid confusion.

@tonyfujs , what do you think?

@tonyfujs, I defer to you this PR. I think it is ready to merge, but I leave it up to you. Also, given my explanation above, do you agree with removing default_ppp or should we leave it?

shahronak47 commented 10 months ago

Also added the end point regression-params for this task https://app.clickup.com/t/86872qh1m . However, it uses pipster package and requires this PR to be approved https://github.com/PIP-Technical-Team/wbpip/pull/236

shahronak47 commented 10 months ago

As discussed, I have copied the functions from pipster and wbpip which are not yet available. The endpoints are now ready to test.

randrescastaneda commented 9 months ago

2.911786

This worked really well.

http://127.0.0.1:8080/api/v1/grouped-stats?welfare=0.0002,0.0006,0.0011,0.0021,0.0031,0.0048,0.0066,0.0095,0.0128,0.0177,0.0229,0.0355,0.0513,0.0689,0.0882&population=0.001,0.003,0.005,0.009,0.013,0.019,0.025,0.034,0.044,0.0581,0.0721,0.1041,0.1411,0.1792,0.2182&requested_mean=2.911786&povline=1.9 image

shahronak47 commented 8 months ago

Hi @randrescastaneda , I have done the changes as per our last discussion :

  1. Include a column called lorenz to define two rows as lq and lb in regression-params endpoint
  2. Change the column name to welfare and weight instead of output and points in lorenz-curve endpoint.
tonyfujs commented 6 months ago

Hey @shahronak47 I am trying to run the most recent branch locally, and I'm getting errors. This is what I tried: http://127.0.0.1:8080/api/v1/grouped-stats?welfare=0.0002,0.0006,0.0011,0.0021,0.0031,0.0048,0.0066,0.0095,0.0128,0.0177,0.0229,0.0355,0.0513,0.0689,0.0882&population=0.001,0.003,0.005,0.009,0.013,0.019,0.025,0.034,0.044,0.0581,0.0721,0.1041,0.1411,0.1792,0.2182&requested_mean=2.911786&povline=1.9

image

shahronak47 commented 6 months ago

Hi @tonyfujs , after discussion with Andres we switched the argument from welfare to cum_welfare and population to cum_population . Sorry for the confusion.

Try using this :

http://127.0.0.1:8080/api/v1/grouped-stats?cum_welfare=0.0002,0.0006,0.0011,0.0021,0.0031,0.0048,0.0066,0.0095,0.0128,0.0177,0.0229,0.0355,0.0513,0.0689,0.0882&cum_population=0.001,0.003,0.005,0.009,0.013,0.019,0.025,0.034,0.044,0.0581,0.0721,0.1041,0.1411,0.1792,0.2182&requested_mean=2.911786&povline=1.9

tonyfujs commented 6 months ago

grouped-stats?cum_welfare=0.0002,0.0006,0.0011,0.0021,0.0031,0.0048,0.0066,0.0095,0.0128,0.0177,0.0229,0.0355,0.0513,0.0689,0.0882&cum_population=0.001,0.003,0.005,0.009,0.013,0.019,0.025,0.034,0.044,0.0581,0.0721,0.1041,0.1411,0.1792,0.2182&requested_mean=2.911786&povline=1.9

Ok. Thanks @shahronak47, and sorry for not responding earlier. It working fine using those parameters. What I would suggest before we merge is to add documentation to the Swagger file for this new point, with a full example so end-users have a concrete example of how to use it. The specs for the Swagger documentation can be edited here: ./inst/plumber/v1/openapi.yaml

tonyfujs commented 6 months ago

Also, @randrescastaneda @shahronak47 can you both confirm that both devtools::test() and devtools::check() pass for you before merging. Some unit tests do not pass for me, but I'm quite sure this has to do with some bad configuration on my laptop, and not with the code itself. Thanks!

shahronak47 commented 6 months ago

Hi @tonyfujs , I have updated the documentation in openapi.yaml file and I confirm that devtools::test() and devtools::check() passes for me with some minor notes.

image

The first one we decided to keep it as it is https://github.com/PIP-Technical-Team/pipapi/issues/402

cc : @randrescastaneda

tonyfujs commented 6 months ago

Great. Thanks @shahronak47 ! All good from my side then.

shahronak47 commented 6 months ago

Thanks @tonyfujs , should we merge this to DEV then @randrescastaneda ?