Skip to content

Introduce error margin by variable in tests#1149

Merged
bonjourmauko merged 3 commits intomasterfrom
error_margin_by_variable
Nov 21, 2022
Merged

Introduce error margin by variable in tests#1149
bonjourmauko merged 3 commits intomasterfrom
error_margin_by_variable

Conversation

@benjello
Copy link
Copy Markdown
Member

@benjello benjello commented Aug 8, 2022

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

New features

  • Introduce variable dependent error margins int YAML tests
    • Allows for different error margin across variables

@benjello benjello self-assigned this Aug 8, 2022
@coveralls
Copy link
Copy Markdown

coveralls commented Aug 8, 2022

Coverage Status

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

@benjello benjello added scope:user-experience contrib:ipp Contributions from IPP type:contrib Contribution from the community (PR) labels Aug 8, 2022
Copy link
Copy Markdown
Contributor

@nikhilwoodruff nikhilwoodruff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks @benjello. This will be useful in both UK and US systems.

@benjello benjello removed request for MattiSG and sandcha August 10, 2022 12:47
@benjello
Copy link
Copy Markdown
Member Author

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
Copy link
Copy Markdown
Member

bonjourmauko commented Aug 21, 2022

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
Copy link
Copy Markdown
Member

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
Copy link
Copy Markdown
Member Author

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 benjello force-pushed the error_margin_by_variable branch from 22fa906 to e79638f Compare August 21, 2022 17:26
@benjello
Copy link
Copy Markdown
Member Author

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

@bonjourmauko
Copy link
Copy Markdown
Member

@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
Copy link
Copy Markdown
Member Author

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

@benjello benjello requested a review from eraviart October 12, 2022 11:38
@MattiSG MattiSG requested a review from bonjourmauko October 12, 2022 14:13
@bonjourmauko bonjourmauko force-pushed the error_margin_by_variable branch from 104b8f0 to b3fa81f Compare November 21, 2022 09:45
@bonjourmauko bonjourmauko force-pushed the error_margin_by_variable branch from b3fa81f to 5ccbf04 Compare November 21, 2022 09:49
@bonjourmauko bonjourmauko force-pushed the error_margin_by_variable branch from 5ccbf04 to ed98b36 Compare November 21, 2022 09:54
@bonjourmauko bonjourmauko merged commit c645aaa into master Nov 21, 2022
@bonjourmauko bonjourmauko deleted the error_margin_by_variable branch November 21, 2022 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contrib:ipp Contributions from IPP scope:user-experience type:contrib Contribution from the community (PR)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants