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

Define functions on scalars and vectorise them #356

Merged
merged 57 commits into from Mar 29, 2022
Merged

Conversation

ChristianZimpelmann
Copy link
Member

@ChristianZimpelmann ChristianZimpelmann commented Mar 14, 2022

What problem do you want to solve?

  • Make functions easier to read and understand
  • Make code much faster

Todo

  • Implement functions to aggregate values on hh or tu level
  • Rewrite all functions such that they are defined on scalars
  • Implement vectorization (for numpy , numba, and jax)
  • Rewrite interface.py such that all columns are on individual level
  • If it is a relevant contribution, put it in CHANGES.rst.

Closes #281

@hmgaudecker hmgaudecker changed the title Transition from functions defined on arrays to functions defined on scalars Define functions on scalars and vectorise them Mar 16, 2022
@codecov
Copy link

codecov bot commented Mar 24, 2022

Codecov Report

❗ No coverage uploaded for pull request base (main@9a626a9). Click here to learn what that means.
The diff coverage is 93.65%.

❗ Current head 0b9338d differs from pull request most recent head a5419b4. Consider uploading reports for the commit a5419b4 to get more accurate results

@@           Coverage Diff           @@
##             main     #356   +/-   ##
=======================================
  Coverage        ?   93.09%           
=======================================
  Files           ?       77           
  Lines           ?     3478           
  Branches        ?        0           
=======================================
  Hits            ?     3238           
  Misses          ?      240           
  Partials        ?        0           
Impacted Files Coverage Δ
gettsim/synthetic_data/synthetic.py 94.54% <ø> (ø)
gettsim/taxes/soli_st.py 100.00% <ø> (ø)
gettsim/tests/test_interface.py 96.66% <ø> (ø)
gettsim/transfers/benefit_checks/benefit_checks.py 100.00% <ø> (ø)
gettsim/visualization.py 52.67% <27.27%> (ø)
gettsim/config.py 85.71% <60.00%> (ø)
gettsim/aggregation.py 67.39% <67.39%> (ø)
gettsim/transfers/unterhalt.py 84.61% <71.42%> (ø)
gettsim/functions_loader.py 85.71% <79.16%> (ø)
gettsim/typing.py 81.25% <80.00%> (ø)
... and 57 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a626a9...a5419b4. Read the comment docs.

@hmgaudecker
Copy link
Collaborator

hmgaudecker commented Mar 28, 2022

Also still needed: Search for "Series" (uppercase) and replace appropriately at least in the GETTSIM functions (not necessary to go in detail through interface.py right now).

There are quite some instances in the docs that were not using "FloatSeries" and friends so far.

@hmgaudecker
Copy link
Collaborator

hmgaudecker commented Mar 28, 2022 via email

@ChristianZimpelmann
Copy link
Member Author

ChristianZimpelmann commented Mar 29, 2022

I'd stick to vermietung and also vor_vermög_check (or the like) — simply
not reason to rename in this PR. Sorry, I see why that was confusing.

I am still not sure what your prefered name is. We have two variables:

  • input variable on monthly basis: current version and version before this PR: eink_vermiet_m
  • defined function which calculates the value on yearly basis: before this PR eink_vermietung, current version: eink_vermiet

Just let me know how you would like to change it.

-17/24 + höchstalter, unless you come up with a really good name (I did
not, but that does not mean anything)

How about altersgrenze? I still think we should, if possible, use the parameter values mentioned in the law.

  • Otherwise, all comments should be addressed now.
  • I haven't implemented the user-provided aggregation-dictionary, yet. Should we postpone?
  • I tried to add python 3.10, but didn't get to run a working conda environment, yet. So let's postpone this for a bit.

@hmgaudecker
Copy link
Collaborator

hmgaudecker commented Mar 29, 2022

I'd stick to vermietung and also vor_vermög_check (or the like) — simply
not reason to rename in this PR. Sorry, I see why that was confusing.

I am still not sure what your prefered name is. We have two variables:

input variable on monthly basis: current version and version before this PR: eink_vermiet_m

defined function which calculates the value on yearly basis: before this PR eink_vermietung, current version: eink_vermiet

Ahh, I did not remember the former. Then we forgot to rename that, I believe. eink_vermietung_m / eink_vermietung, if that is not too much of a hassle

How about altersgrenze? I still think we should, if possible, use the parameter values mentioned in the law.

Very good.

Otherwise, all comments should be addressed now.

Perfect!

I haven't implemented the user-provided aggregation-dictionary, yet. Should we postpone?

Sure, just open an issue. Or a PR directly, should be just a couple of hours' work, right? Then would be nice to have in release.

I tried to add python 3.10, but didn't get to run a working conda environment, yet. So let's postpone this for a bit.

Sure.

Copy link
Collaborator

@hmgaudecker hmgaudecker left a comment

Choose a reason for hiding this comment

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

Excellent!

@hmgaudecker hmgaudecker merged commit dda2e43 into main Mar 29, 2022
@hmgaudecker hmgaudecker deleted the scalar_trans branch March 29, 2022 08:33
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.

Automate aggregation of individual measures
3 participants