Improve age computation formula and Variables tutorial comments #43
Conversation
|
@fpagnoux CircleCI's flake8 complaint seems a bit strange to me, since it is related to the age formula you wrote would you mind handling it? 🙂
|
| # This file defines the variables of our legislation. | ||
| # A variable is property of a person, or an entity (e.g. a household). | ||
| # This file defines variables for the modelled legislation. | ||
| # A variable is a property of an Entity such as a Person, a Family… |
There was a problem hiding this comment.
Shouldn't we use Household? There is no Family in this model.
| from openfisca_country_template.entities import * | ||
|
|
||
| # Import additional NumPy helpers to work on specific data types | ||
| from numpy import datetime64 |
There was a problem hiding this comment.
I don't think we are actually using this import anymore, and that's why Flake is complaning.
There was a problem hiding this comment.
I agree, but I wanted a double check from the person who wrote the initial code 😉
| return (datetime64(period.date) - birth).astype('timedelta64[Y]') | ||
| birth_year = birth.astype('datetime64[Y]').astype(int) + 1970 | ||
| birth_month = birth.astype('datetime64[M]').astype(int) % 12 + 1 | ||
| birth_day = (birth - birth.astype('datetime64[M]') + 1).astype(int) |
There was a problem hiding this comment.
Frankly ugly, but we don't have better yet
There was a problem hiding this comment.
I'd really like that a time helper library is researched and vetted for.
There was a problem hiding this comment.
We started to ask around, and there are candidates (Maya, Arrow, etc.). Though in this specific case, we need to handle vectors of dates, which will be a clear limitation.
If we were to use a library, we should also probably also use it to handle periods. So IMO this is related to openfisca/openfisca-core#670.
|
@fpagnoux the tests seem to show there is an issue with the formula 😕 We should probably add more tests to this in any case, including for leap years. |
Indeed. I fixed a formula and added a test for leap years. Feel free to add the tests you'll find relevant 🙂. |
|
That was an annoying rebase, hopefully I got it right 😉 |
(just kidding, I double-checked that the diff from the resulting rebase to the previous branch was equal to the changes made to |
demographics.ageageformulaComments improvements are only partly a knight: PRs such as ServiceInnovationLab/openfisca-aotearoa#15 show that reusers will copy and paste comments and that can lead to review friction.