search

openfisca / openfisca-core

OpenFisca core engine. See other repositories for countries-specific code & data.
https://openfisca.org
GNU Affero General Public License v3.0
168 stars 75 forks source link

Introduce error margin by variable in tests #1149

Closed benjello closed 1 year ago

benjello commented 2 years ago

Thanks for contributing to OpenFisca! Remove this line, as well as any other in the following that don't fit your contribution :)

New features

coveralls commented 2 years ago

Coverage Status

Coverage increased (+0.03%) to 79.249% when pulling ed98b3619f672c28c31cbaadca849bb1eee00770 on error_margin_by_variable into ea57e8e5044e0cca6f867446df98e9dbee0c1817 on master.

benjello commented 2 years ago

Doc building fails and should be fixed by #1145.

@maukoquiroga and @MattiSG please ping me when the later is merged so I can move on with this PR. Thanks !

bonjourmauko commented 2 years ago

Hi @benjello ! Thanks for your contribution. Could you please write a couple of lines so I or an other reviewers can understand the motivation for this PR? Just

  1. Need
  2. Complication to solve the need with current code base
  3. What you propose and why you propose it this way and not another way

Quickly from reading the code this is what I got:

  1. We already accept calculation level error margins, which are absolute.
  2. However, because quality of data is not always homogeneous, in some cases, outlier errors in one variable calculation makes the whole calculation to fail. Which is generally OK except when we want to observe one variable in isolation.
  3. You propose to define a. A calculation-level error margin that is called absolute in terms of the desired result. b. A variable-level error margin that is relative to the absolute one and the desired result. c. The default error is absolute and is a scalar, e.g. 100 €. d. The relative error is relative to the default and is a percentage, e.g. 5%.

Nota bene: I'm intrigued as we have three kinds of logic in OpenFisca that mix up: data-objects (entities, periods, enums...), modelling (parameters, formulas, variables...), and the intricacies of actual calculations (simulations, tax scales, error margins...). IMHO, if the latter is needed by the community they should be first-class citizens, but also be appropriately encapsulated, which isn't the case today.

bonjourmauko commented 2 years ago

This goes beyond the PR because error margin logic already exist in OF, but to be clear, when we say income tax has a relative margin error of 5%, we're saying just that income tax can have an absolute margin error of +- result * relative error margin right?

benjello commented 2 years ago

This goes beyond the PR because error margin logic already exist in OF, but to be clear, when we say income tax has a relative margin error of 5%, we're saying just that income tax can have an absolute margin error of +- result * relative error margin right?

See above for the definition of relative error margin

benjello commented 2 years ago

@maukoquiroga : does my answer clarify the PR. May be we can make it clearer in the doc after merging the PR.

bonjourmauko commented 2 years ago

@maukoquiroga : does my answer clarify the PR. May be we can make it clearer in the doc after merging the PR.

Yes, I understand better.

The reason I ask all this questions is to make it easier for any reviewer out there to get the logic and review the code, and eventually accept/reject the proposal.

I certainly have no grounds to reject it now that I understand the value it adds, however I wasn't able to review the actual implementation of it without that understanding.

I hope to give you a proper review soon if nobody gives you one before.

benjello commented 1 year ago

@nikhilwoodruff @maukoquiroga : I do need a final approval to merge this PR ;-)