-
Notifications
You must be signed in to change notification settings - Fork 31
Create bg_id and wthh_id endogenously. #768
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just skimmed through it, great start!
| --- | ||
| info: | ||
| note: '' | ||
| note: Skipped because multiple fgs in hh. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this statement is wrong here and for the next few tests, so not repeating. Sometimes there are multiple households indeed, but only one fg per household.
| - true | ||
| - true | ||
| assumed: {} | ||
| assumed: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not make it the same as in the next few tests, i.e., adding einstandspartner etc? No big deal, but might make people wonder who look at these one after the other.
| info: | ||
| note: https://www.bmwsb.bund.de/Webs/BMWSB/DE/themen/wohnen/wohnraumfoerderung/wohngeld/wohngeldrechner-2023-artikel.html | ||
| source: '' | ||
| note: Skipped because multiple fgs in hh. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we supply the bg_id or would that run counter the purpose of the test in the first place? Same for the next test.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #768 +/- ##
==========================================
- Coverage 89.62% 89.41% -0.21%
==========================================
Files 52 52
Lines 3768 3780 +12
==========================================
+ Hits 3377 3380 +3
- Misses 391 400 +9 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make 9 tests out of this one and similar ones, else the yaml is illegible. Apologies, I did not mean my suggestion elsewhere to be taken that literally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure? I worked with both versions and as this is mainly about when household members switch BGs / wthh, I think it helps to have households that are similar (=everything the same, only wage is increasing across households) together and ordered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! I think this is a great example of where the current thing makes sense while developing (you are sure about the structure and unsure what is correct), but does not when some test starts to fail in 3 years from now (you'd look at the huge test file like I did a couple of hours ago and it takes you ages to realise the precise structure because that's not what our yaml structure is made for).
So feel free to change only when this is ready for review.
Structure should be clear when you nest one level deeper:
endogenous_bg_id/2023/single_parent_unterhalt_1000_1_child/
- parental_income_0.yaml
- parental_income_520.yaml
etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, makes sense!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just realised it is confusing to have the two digits following each other in the directory name. Maybe change to:
endogenous_bg_id/2023/single_parent_1_child_unterhalt_1000/
- parental_income_0.yaml
- parental_income_520.yaml
? Also more sensible to have number of children first and then the alimony paid for them.
|
Improved version in #778 |
What problem do you want to solve?
Create the bg_id and wthh_id for households with exactly 1
fgendogenously.Based on the discussion in #763