Closed nafetsk closed 10 months ago
All modified and coverable lines are covered by tests :white_check_mark:
Comparison is base (
1da4d57
) 91.01% compared to head (2d60366
) 91.02%.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
We might have discussed that before, but it would seem more natural to me to have separate variables entgeltpunkte_ost
and entgeltpuntkte_west
rather than the total and fractions. Any specific reasons why you picked the current version?
Also, how do you plan to implement the accumulation of points? Based on place of residence?
Would be good to make a detailed list in the PR first on what you plan to do (or discuss on Zulip), so one can have an early discussion. Some things one will only see when in code, but many things can be cleared up ex ante.
We might have discussed that before, but it would seem more natural to me to have separate variables
entgeltpunkte_ost
andentgeltpuntkte_west
rather than the total and fractions. Any specific reasons why you picked the current version?Also, how do you plan to implement the accumulation of points? Based on place of residence?
Would be good to make a detailed list in the PR first on what you plan to do (or discuss on Zulip), so one can have an early discussion. Some things one will only see when in code, but many things can be cleared up ex ante.
We thought a share variable would make sense, as there is one in SOEP-RV (OPXAZ).
In the first draft, I had implemented a function that adjusts the share variable depending on the place of residence and the EGPT earned. However, we then took it out again because we thought that the accumulation of EGPT over time was hardly implemented and that this should be a separate PR if at all.
And it's true that we should announce PRs earlier and put them up for discussion
Thanks for the explanation!
The SOEP is irrelevant to the extent that it should not drive implementation details. It's trivial to calculate two levels from total + share outside of GETTSIM.
However, we then took it out again because we thought that the accumulation of EGPT over time was hardly implemented and that this should be a separate PR if at all.
Not sure why you are saying that? There is entgeltp_update
, which of course is missing lots of things, but the latter is irrelevant to the question at hand.
Hi, thanks for the feedback! I agree that using stocks vs. shares east shouldn't make much of a difference. We went for shares since this is what is most widely available in the datasets we used so far and it's the most straightforward way to add EGPT from Zurechnungszeit. Thinking about an updating functionality though, using two separate stocks is probably more simple.
Regarding the updating function: I argued for not adding it for now to keep this change is minimal as possible, since I though more "dynamic" functionalities of GETTSIM will be part of larger future PRs. I am completely fine with adding it again though, if this makes more sense!
But the function is there? With two stocks, it will be obvious it'll need updating.
But the function is there? With two stocks, it will be obvious it'll need updating.
Yes thinking about it more now I agree, we will add it back then :-)
Hey, got rid of the share variabel and introduced entgeltp_west
and entgeltp_ost
as new input variables. In order to update them I replaced entgeltp_update
with entgeltp_ost_update
and entgeltp_west_update
.
Very nice! Just stylistic issues / trivialities left.
One more relevant thing: As trivial as they may be, it would be nice to add a couple of tests on the two entgeltp_* - functions directly.
The testcases in test_renten_anspr
are checking both update functions. I could add another case that checks for a mixed earning point biography. Is that enough or should I rather create a new test for that?
The testcases in
test_renten_anspr
are checking both update functions. I could add another case that checks for a mixed earning point biography. Is that enough or should I rather create a new test for that?
Ah, I did not realise that. As long as both east and west are checked, all is fine.
The testcases in
test_renten_anspr
are checking both update functions. I could add another case that checks for a mixed earning point biography. Is that enough or should I rather create a new test for that?Ah, I did not realise that. As long as both east and west are checked, all is fine.
I added another test case just to make sure it works for a mixed (east/west) biography. Tests should be sufficient now.
What problem do you want to solve?
Distinction between East and West earning points in the calculation of the pension. Differentiation possible via a new input variable called "anteil_entgeltp_ost", which is equivalent to the OPXAZ variable of the SOEP-RV.
Todo
Closes #XXXX
in the first PR comment to auto-close the relevant issue once the PR is accepted. This is not applicable if there is no corresponding issue.