This PR fixes case in SS of baseline=True and baseline_spending=True

[rickecon]

This PR solves a problem that was identified in Issue #782. The case when baseline=True and baseline_spending=True was not correctly treated in the code and was not tested anywhere in the unit testing files.

This PR will be ready for review once I confirm that the TPI works correctly in this case and I have included tests for this case.

cc: @jdebacker

[codecov-commenter]

Codecov Report

Merging #786 (393b5f6) into master (7f8f384) will decrease coverage by 0.03%.
The diff coverage is 16.66%.

@@            Coverage Diff             @@
##           master     #786      +/-   ##
==========================================
- Coverage   86.36%   86.32%   -0.04%     
==========================================
  Files          40       40              
  Lines        6314     6318       +4     
==========================================
+ Hits         5453     5454       +1     
- Misses        861      864       +3     

Flag	Coverage Δ
unittests	86.32% <16.66%> (-0.04%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
tests/test_SS.py	76.19% <0.00%> (-0.31%)	⬇️
ogcore/parameters.py	86.36% <33.33%> (-1.24%)	⬇️

[rickecon]

@jdebacker. In Issue #782, you note that baseline_spending=True is only valid in the reform case (baseline=False) and that we cannot condition baseline_spending=True to have that requirement of baseline=False in ParamTools validation because the baseline parameter often changes within a script that performs a baseline and reform. @jdebacker proposed:

but we can put an assert statement somewhere that doesn't allow this combination of parameter values.

This PR is an alternative to the assert statement approach. To do the assert statement approach, we have to make sure we assert that baseline_spending == True and baseline == False at every entry point to the model.

The approach of this PR allows the baseline_spending parameter to be more of a flag. Every if gate instance of p.baseline_spending == True is now accompanied by and not p.baseline. This allows a user to erroneously set baseline_spending==True during the baseline run and to have the feature apply correctly to only the reform.

I am almost done running the full set of unit tests on my machine for this PR. But I am happy to go the assert statement route if you want. Awaiting discussion.

[jdebacker]

@rickecon writes:

To do the assert statement approach, we have to make sure we assert that baseline_spending == True and baseline == False at every entry point to the model.

One can do this assert approach by adding the following below line 35 in parameters.py (maybe making the message more clear/descriptive):

if baseline:
   assert not p.baseline_spending, "Cannot use baseline spending in a run of the model baseline"

If this is done, no other changes to the code should be necessary. Though perhaps a better description of this issue with the meta data for the baseline_spending parameter in default_parameters.json is warranted (as suggested by Martin).

[rickecon]

I ran the full set of CI tests locally on my machine and had two failures in the test_SS.py module in the test_run_SS test function in specifications [Reform, use zeta] and [Reform, small open use zeta]. Both errors were RuntimeError: Steady state equilibrium not found, which is probably easily fixed with an adjusted starting value for initial_guess_r_SS.

ogcore/tests/test_SS.py ...........................F.F..           [  7%]
ogcore/tests/test_TPI.py ........................                  [ 12%]
ogcore/tests/test_aggregates.py ................................   [ 20%]
ogcore/tests/test_basic.py ....                                    [ 21%]
ogcore/tests/test_elliptical_u_est.py .......                      [ 22%]
ogcore/tests/test_execute.py .                                     [ 22%]
ogcore/tests/test_firm.py .......................................  [ 31%]
ogcore/tests/test_fiscal.py ..................                     [ 35%]
ogcore/tests/test_household.py ........................................  [ 45%]
ogcore/tests/test_output_plots.py ...................................... [ 53%]
ogcore/tests/test_output_tables.py .............                         [ 56%]
ogcore/tests/test_parameter_plots.py ................................... [ 64%]
ogcore/tests/test_parameter_tables.py .......                   [ 66%]
ogcore/tests/test_parameters.py .............                   [ 69%]
ogcore/tests/test_run_example.py ..                             [ 69%]
ogcore/tests/test_run_ogcore.py .                               [ 70%]
ogcore/tests/test_tax.py ...............................        [ 77%]
ogcore/tests/test_txfunc.py .........................           [ 82%]
ogcore/tests/test_user_inputs.py .........                      [ 84%]
ogcore/tests/test_utils.py ................................................................ [ 99%]
ogcore/tests/test_wealth.py ..                                  [100%]

The change in the most recent commit to test_SS.py::test_run_SS[Reform, use zeta] changes the initial guesses for r_ss and TR_SS in param_updates6 to 'initial_guess_r_SS': 0.09 and 'initial_guess_TR_SS': 0.05. This solves the first failure.

The second failure was more tricky. I tried all the permutations of initial_guess_r_SS = [0.05, 0.06, 0.07, 0.08, 0.09, 0.10] and initial_guess_TR_SS = [0.03, 0.04, 0.05] in param_updates8 in the test_SS.py::test_run_SS[Reform, small open use zeta] test. None of these solved, and each ended with the same error RuntimeError: Steady state equilibrium not found. I am at a loss of how to solve this second failure.

[rickecon]

@jdebacker. The question in this PR is whether to allow a user to specify baseline_spending=True as a flag or whether to require it to only be used in conjuction with the baseline=False. If we prefer the latter, then we should just go the "assert statement in parameters.py" route. Otherwise, the model code in the current PR allows the user to specify baseline_spending=True as a flag that can be in place during the baseline=True stage, even though it only applies to baseline=False. I like this second approach. One reason is for cases like in Issue #786. Another reason is that I like the flag definition and interpretation of baseline_spending. A final reason is that I think the use cases are spelled out more explicitly in the code. Happy to discuss.

[jdebacker]

@rickecon FYI, all SS tests pass for me on master branch:

(ogcore-dev) jason.debacker@JDEBACKER-2 OG-Core % pytest ogcore/tests/test_SS.py

============================= test session starts ==============================
platform darwin -- Python 3.10.2, pytest-7.0.0, pluggy-1.0.0
rootdir: /Users/jason.debacker/repos/OG-Core, configfile: pytest.ini
plugins: xdist-2.5.0, forked-1.4.0
collected 32 items
ogcore/tests/test_SS.py ................................                 [100%]
================= 32 passed, 23 warnings in 2571.40s (0:42:51) =================
(ogcore-dev) jason.debacker@JDEBACKER-2 OG-Core %

[rickecon]

@jdebacker. This PR is almost ready for review. The errors that I was finding in test_SS.py have gone away. They were somehow an artifact of the code changes I had made previously, although I don't understand how those code changes made those tests fail. I did run all the test_SS.py tests in the master branch, and they all passed.

ogcore/tests/test_SS.py ................................                 [100%]

I re-ran the entire set of CI tests locally last night and got the following output, with only two errors in test_SS.py.

ogcore/tests/test_SS.py .....................F.F........                                     [  7%]
ogcore/tests/test_TPI.py ........................                                            [ 12%]
ogcore/tests/test_aggregates.py ................................                             [ 20%]
ogcore/tests/test_basic.py ....                                                              [ 21%]
ogcore/tests/test_elliptical_u_est.py .......                                                [ 22%]
ogcore/tests/test_execute.py .                                                               [ 22%]
ogcore/tests/test_firm.py .......................................                            [ 31%]
ogcore/tests/test_fiscal.py ..................                                               [ 35%]
ogcore/tests/test_household.py ........................................                      [ 45%]
ogcore/tests/test_output_plots.py ......................................                     [ 53%]
ogcore/tests/test_output_tables.py .............                                             [ 56%]
ogcore/tests/test_parameter_plots.py ...................................                     [ 64%]
ogcore/tests/test_parameter_tables.py .......                                                [ 66%]
ogcore/tests/test_parameters.py .............                                                [ 69%]
ogcore/tests/test_run_example.py ..                                                          [ 69%]
ogcore/tests/test_run_ogcore.py .                                                            [ 70%]
ogcore/tests/test_tax.py ...............................                                     [ 77%]
ogcore/tests/test_txfunc.py .........................                                        [ 82%]
ogcore/tests/test_user_inputs.py .........                                                   [ 84%]
ogcore/tests/test_utils.py ................................................................  [ 99%]
ogcore/tests/test_wealth.py ..                                                               [100%]

The two tests failing in test_SS.py are test_run_SS[Reform, baseline spending] and test_run_SS[Reform, baseline spending, use zeta]. Both of these tests set baseline_spending=True. But the code in the test_run_SS() test sets baseline=True which causes the error ValueError: Parameter baseline_spending=True cannot coincide with baseline=True. The issue is that a baseline_spending=True run requires baseline=False and requires baseline output to be saved in an OUTPUT_BASELINE folder. In this test, the OUTPUT_BASELINE directory is now a tmpdir from PR #775.

I think the current tests in the master branch which are all passing are not testing what we expect. They were cheating by setting baseline=True which would negate the baseline_spending=True setting. To really test baseline_spending=True, we need to set up a different test that runs a baseline first, then the reform so that any tmpdirs are not erased after the baseline run. The current offending code in test_SS.py::test_run_SS is in lines 509-518.

baseline_dir = os.path.join(tmpdir, 'OUTPUT_BASELINE')
if baseline is False:
    p_base = Specifications(
        output_base=baseline_dir,
        baseline_dir=baseline_dir,
        baseline=True,
        num_workers=NUM_WORKERS)
    p_base.update_specifications(param_updates)
    p_base.baseline_spending = False
    base_ss_outputs = SS.run_SS(p_base, client=dask_client)

I will work on making this change. Then this PR will be ready.

[rickecon]

@jdebacker. This PR is now ready for your review. I have changed its "Draft" status to "Ready for review".

• I merged in PR Move test files #788, that moves all the test files.
• I made the small change to test_SS.py to allow for the baseline_spending=True case.
• I ran all the tests on test_SS.py, and they all work.

tests/test_SS.py ................................  [100%]

[jdebacker]

Excellent - thanks @rickecon!