Fix and refactor AMT

[PavelMakarchuk]

Fixes #4775

[PavelMakarchuk]

This is the piece that was added in this PR

[martinholmer]

Can you explain why regular_tax_before_credits is being added to itself?
What am I missing?

[MaxGhenis]

is this resolved?

[PavelMakarchuk]

Yes

[martinholmer]

@PavelMakarchuk, in PR #4815 the new code looks like this:

I don't understand line 122: the capital_gains_tax is not being assigned to a local variable.
What's the purpose of line 122?

And on line 124, I don't understand the point of adding regular_tax_before_credits to itself?

Should the new code be something like this?

        tax_before_credits = (
            regular_tax_before_credits + tax_unit("capital_gains_tax", period)
        )

[PavelMakarchuk]

@PavelMakarchuk, in PR #4815 the new code looks like this:

I don't understand line 122: the capital_gains_tax is not being assigned to a local variable. What's the purpose of line 122?

And on line 124, I don't understand the point of adding regular_tax_before_credits to itself?

Should the new code be something like this?

        tax_before_credits = (
            regular_tax_before_credits + tax_unit("capital_gains_tax", period)
        )

Yes, my mistake, that was exactly the intention

[martinholmer]

@PavelMakarchuk, There seems to be a problem with building the documentation.
What's that all about?

[PavelMakarchuk]

@PavelMakarchuk, There seems to be a problem with building the documentation. What's that all about?

The CI issue is affecting every Pull Request right now - we have an issue filed for it but it has not been fixed yet

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.00%. Comparing base (daf7988) to head (a21be3e).
Report is 76 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4815      +/-   ##
==========================================
+ Coverage   98.98%   99.00%   +0.01%     
==========================================
  Files        2517     2532      +15     
  Lines       36615    36801     +186     
  Branches      197      152      -45     
==========================================
+ Hits        36245    36435     +190     
+ Misses        339      337       -2     
+ Partials       31       29       -2     

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

[MaxGhenis]

Seems like we're hitting some sort of resource limit on the GH Actions servers, around the microsim tests. I don't think this PR is adding significant resource use, but it could be a tipping point.

Some options I see include:

1. Trim down the microsim tests
2. Improve efficiency, e.g. using more defined_fors or caching
3. Break up the test suite (not sure how resource constraints are handled / if this would help)
4. Pay for larger runners

@nikhilwoodruff any other options you'd see?

[PavelMakarchuk]

Seems like we're hitting some sort of resource limit on the GH Actions servers, around the microsim tests. I don't think this PR is adding significant resource use, but it could be a tipping point.

Some options I see include:

1. Trim down the microsim tests
2. Improve efficiency, e.g. using more defined_fors or caching
3. Break up the test suite (not sure how resource constraints are handled / if this would help)
4. Pay for larger runners

@nikhilwoodruff any other options you'd see?

I can remove Microsim tests (mostly not entirely) if we agree that this is the solution that we are going for

[MaxGhenis]

Sure, try removing one or two to start?

Does make test work for you locally?

[PavelMakarchuk]

Seems like we're hitting some sort of resource limit on the GH Actions servers, around the microsim tests. I don't think this PR is adding significant resource use, but it could be a tipping point.
Some options I see include:

1. Trim down the microsim tests
2. Improve efficiency, e.g. using more defined_fors or caching
3. Break up the test suite (not sure how resource constraints are handled / if this would help)
4. Pay for larger runners

@nikhilwoodruff any other options you'd see?

I can remove Microsim tests (mostly not entirely) if we agree that this is the solution that we are going for

I would also think the reducing the test suite for structural reforms could help

[PavelMakarchuk]

Sure, try removing one or two to start?

Does make test work for you locally?

The make test passes locally