Skip to content

Conversation

@MImmesberger
Copy link
Collaborator

Closes #690

@lars-reimann
Copy link
Contributor

Regarding this comment:

@lars-reimann Can you have a look where the CondaVerificationError in the readthedocs check comes from?

I could not reproduce this locally. It seems to be caused by the upgrade of sphinx-automodapi to version 0.17.0. In previous, successful runs version 0.16.0 was used. Maybe you can add an upper bound for sphinx-automodapi in environment.yml in this PR and try again?

- sphinx-automodapi<0.17.0

@lars-reimann
Copy link
Contributor

That seems to have fixed the install issue at least.

@MImmesberger
Copy link
Collaborator Author

Yes, I guess the other errors are due to this incomplete PR. Thanks!

@lars-reimann lars-reimann force-pushed the 690-remove-tu-groupings branch 2 times, most recently from a8b9bd1 to 47745f4 Compare March 4, 2024 10:33
@lars-reimann lars-reimann force-pushed the 690-remove-tu-groupings branch from 0d878ea to 91b4b9c Compare March 4, 2024 10:41
@lars-reimann lars-reimann force-pushed the 690-remove-tu-groupings branch from cc80cba to 4df035d Compare March 4, 2024 10:46
@lars-reimann lars-reimann force-pushed the 690-remove-tu-groupings branch from c9784bf to 4a58c8e Compare March 4, 2024 11:42
@lars-reimann lars-reimann force-pushed the 690-remove-tu-groupings branch from 3de50f0 to 6ff5618 Compare March 4, 2024 11:48
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.

Just a couple of remarks while browsing the changes.

@MImmesberger
Copy link
Collaborator Author

I don't think there are particular tests you have to look at.

Just so it doesn't sneak through the review process: In 1d78ded, I set the minimum relevant income for ALG2 to 0. There were some test cases that resulted in negative values after getting rid of tu.

@MImmesberger
Copy link
Collaborator Author

The rounding seems to be an issue when switching from eink_st_y_sn / 12 to eink_st_m_sn, because rounding parameters are expected for the eink_st_m_sn key in the param yaml. They are specified for the eink_st_y_sn key only.

I'm not entirely sure what the expected behavior should be:

  1. First round and then time convert (with optional rounding spec for converted column).
  2. First time convert and then round (We could use a quick fix by specifying the eink_st_m_sn rounding spec in the params yaml).

Because eink_st_m_sn is used as an approximation for Lohnsteuer (at least for Elterngeld and Grundsicherung; not sure about ALG2) we cannot infer expected behavior from the law.

@hmgaudecker
Copy link
Collaborator

Just so it doesn't sneak through the review process: In 1d78ded, I set the minimum relevant income for ALG2 to 0. There were some test cases that resulted in negative values after getting rid of tu.

Cool, thanks for the pointer. In principle, I think that ALG II should guarantee a consumption floor, but it might be that the mechanism for that is more complicated. I would have no idea. But I'd kind of like to guarantee that subsistence level (thinking of running GETTSIM on models that do require a positive consumption), unless the law is different.

If possible, I'd prefer to revert and make an issue that we'll need to investigate this case.

@hmgaudecker
Copy link
Collaborator

The rounding seems to be an issue when switching from eink_st_y_sn / 12 to eink_st_m_sn, because rounding parameters are expected for the eink_st_m_sn key in the param yaml. They are specified for the eink_st_y_sn key only.

I'm not entirely sure what the expected behavior should be:

1. First round and then time convert (with optional rounding spec for converted column).

2. First time convert and then round (We could use a quick fix by specifying the `eink_st_m_sn` rounding spec in the params yaml).

Because eink_st_m_sn is used as an approximation for Lohnsteuer (at least for Elterngeld and Grundsicherung; not sure about ALG2) we cannot infer expected behavior from the law.

Behavior 1. and the last two cases we should get rid of eventually now that we have Lohnsteuer.

@MImmesberger
Copy link
Collaborator Author

The rounding seems to be an issue when switching from eink_st_y_sn / 12 to eink_st_m_sn, because rounding parameters are expected for the eink_st_m_sn key in the param yaml. They are specified for the eink_st_y_sn key only.
I'm not entirely sure what the expected behavior should be:

1. First round and then time convert (with optional rounding spec for converted column).

2. First time convert and then round (We could use a quick fix by specifying the `eink_st_m_sn` rounding spec in the params yaml).

Because eink_st_m_sn is used as an approximation for Lohnsteuer (at least for Elterngeld and Grundsicherung; not sure about ALG2) we cannot infer expected behavior from the law.

Behavior 1. and the last two cases we should get rid of eventually now that we have Lohnsteuer.

@lars-reimann Can you have a look?

@hmgaudecker
Copy link
Collaborator

@lars-reimann Can you have a look?

Would be great, but let's do that separately from here. Apologies if that means reverting something I noted. Also fine to skip a couple of tests for the moment.

@MImmesberger
Copy link
Collaborator Author

Cool, thanks for the pointer. In principle, I think that ALG II should guarantee a consumption floor, but it might be that the mechanism for that is more complicated. I would have no idea. But I'd kind of like to guarantee that subsistence level (thinking of running GETTSIM on models that do require a positive consumption), unless the law is different.

I think we are talking about different things here. I realized that the issue I raised is solved once we fix #606 because it makes sure that we don't compute negative "anrechenbares Erwerbseinkommen" on the individual level.

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.

Great work!!! Thanks a ton.

"""

if eltern:
if erwachsen and (not kindergeld_anspruch):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you make a note of this somewhere? My point is that we simply should think about some value other than zero for people who do not have potentially eligible children. Probably tackle in #704, but we should remember explicitly to change this condition.

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.

BUG: Replace _tu grouping

4 participants