Skip to content
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

GEP 6 #471

Draft
wants to merge 32 commits into
base: main
Choose a base branch
from
Draft

GEP 6 #471

wants to merge 32 commits into from

Conversation

hmgaudecker
Copy link
Collaborator

What problem do you want to solve?

This GEP outlines a unified architecture for GETTSIM based on a DAG that encompasses:

  1. Namespaces for (policy) functions.
  2. Decorators for including or excluding functions from the DAG, e.g., based on points
    in time.
  3. Pre-processing of parameters.

Implementing this structure will make GETTSIM much more flexible and more natural to use
/ extend.

@hmgaudecker hmgaudecker marked this pull request as draft December 22, 2022 14:23
@codecov
Copy link

codecov bot commented Dec 22, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2451961) 91.31% compared to head (6ec9760) 91.19%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #471      +/-   ##
==========================================
- Coverage   91.31%   91.19%   -0.13%     
==========================================
  Files          48       49       +1     
  Lines        3225     3315      +90     
==========================================
+ Hits         2945     3023      +78     
- Misses        280      292      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@ChristianZimpelmann ChristianZimpelmann left a comment

Choose a reason for hiding this comment

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

This looks very promising! A few remarks:

  • I like the dates_active decorators a lot. I assume they won't be able to handle the renaming of functions (e.g. Arbeitslosengeld II -> Bürgergeld)?
  • I don't fully understand yet how the input data would look like. Do the columns need to be assigned to name spaces? Do the column names have to be something like arbeitsl_geld___anwartschaftszeit and doesn't this break the possibility to call GETTSIM within Stata?
  • For me it is not yet clear how the new structure would handle the pre-processing of derived parameters (e.g. Soli 1.upper_threshold value is not a parameter #444)
  • A disadvantage of the use of namespaces would be that functions would break when moved to a different module. Isn't this very unusal?

docs/geps/gep-06.md Outdated Show resolved Hide resolved
docs/geps/gep-06.md Outdated Show resolved Hide resolved
docs/geps/gep-06.md Outdated Show resolved Hide resolved
@hmgaudecker
Copy link
Collaborator Author

Excellent points, thank you!

I like the dates_active decorators a lot. I assume they won't be able to handle the renaming of functions (e.g. Arbeitslosengeld II -> Bürgergeld)?

Not directly. We should think about whether on top of the remove_suffix keyword, a change_name would be useful.

Note that this would be for functions within a namespace, Arbeitslosengeld II -> Bürgergeld would actually be at the namespace level. In that case, I can't see a better way than copying the parameter files and explicitly importing functions that can be re-used from the former namespace.

I don't fully understand yet how the input data would look like. Do the columns need to be assigned to name spaces? Do the column names have to be something like arbeitsl_geld___anwartschaftszeit and doesn't this break the possibility to call GETTSIM within Stata?

See c1a48f4

For me it is not yet clear how the new structure would handle the pre-processing of derived parameters (e.g. #444)

See 5c36698

A disadvantage of the use of namespaces would be that functions would break when moved to a different module. Isn't this very unusal?

Yes and no. Note the way I wrote it up, namespaces are not at the module, but at the directory level. So if you move a function from, say, ges_rente to arbeitsl_v, it will expect the same parameters / columns from its new namespace unless you explicitly set them. Which I would think about as a feature, not a bug.

@ChristianZimpelmann
Copy link
Member

Thanks for the clarifications and adjustments. I deleted one of your two comments (which were duplicated).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants