Refactoring checkConditions()

[sbenthall]

This PR addresses issue #542

First commit:

• remove 'public_call' argument from checkConditions(); redundant wit verbose

[llorracc]

The public_call indicator was supposed to be set to True whenever checkConditions was invoked by someone directly using it interactively from the command line. The point was that if you are running it by hand at the command line you should get the full set of verbose results. Does this modification preserve that functionality?

[sbenthall]

No, it does not.
I don't think the code before this PR did that either though. public_call was False by default.

It is possible that this functionality changed in my earlier refactoring of this in a prior PR.
These kinds of side-effects are difficult to test. See #525

It remains the case that public_call was redundant with verbose no?
I'd imagine the right way to do this would be to have the functionality you want work with the verbose argument. Why is an additional one necessary?

Separately, @llorracc , I hope you'll take a look at this commit, which is a proposed refactoring of the condition logic:
35808ce

[sbenthall]

@llorracc I recommend we continue discussion of the public_call option on issue #283, as it is tied up with other design decisions brought up there.

[sbenthall]

Likewise with 413bef1, @llorracc

The motivation here is:

• more code reuse for checking Conditions by functionalizing out the key logic. This makes the code more succinct (you can see this PR reduces the lines of code by 30)
• using conditions as a dictionary namespace for tested conditions, which puts these important variables in a place where (a) they are easy to find (b) they won't be accidentally overwritten
• disentangles the specific print statements from the computational logic. This is a first step towards wrapping these in a more flexible logging/UI logic.

I know that this code addresses/expresses some of your substantive research contributions, and so I expect you are sensitive to changes here.

What I'm working towards here is trying to figure out the general feature that this is a special case of.
How would "checkConditions" be used in models that a new user might develop?

[sbenthall]

The merge conflicts have no been fixed.
This PR now addresses one of four issues raised in #596

[sbenthall]

@llorracc you mentioned on the call today an error with this PR concerning GICPF or something.
I haven't been able to find where it's mentioned in an issue.

[llorracc]

@sbenthall, on my screen I see my own question/comment immediately above your query about where my question/comment might be. Maybe you are seeing it differently? Anyway, the point was that I'd changed the 'GIC' to 'GICPF' because this was the version of the GIC that applied to the Perfect Foresight model; but your edit reverted to the old 'GIC' terminology, and I wanted you to change it to 'GICPF'

[sbenthall]

That's very weird, us seeing the issue differently.
In any case, I'll fix this.

[sbenthall]

Ok, I have looked over the code in this PR. I think you have mischaracterized what's happening in it.

• 'GICPF', via checkGICPF() is still used in the PerfForeseightConsumerType
• 'GIC' is used via checkGICInd() in IndShockConsumerType

[llorracc]

I'll look this over and merge if it seems to work.

[sbenthall]

The failing builds look to be due to something in GitHub's internals/backend.
GitHub seems to be undergoing some kind of maintenance--GitHub Pages was down this morning.
I'll try rerunning the jobs again tomorrow.

[sbenthall]

See #542 (comment)

[sbenthall]

@llorracc the most recent commit resolves the issues that came up in your review.

The issue of the different conditions having different meaning is not yet resolved in this PR, but seems to be an extension of existing functionality:
#542 (comment)

If you need this refactor in before your BufferStock publication, this PR can be merged in as is.

That may be wisest, since checking the conditions for a steady-state ergodic distribution seems to naturally connect to the upcoming work on seeding the simulation with the ergodic distribution. It would be best not to block the Bufferstock publication on this new work.

[llorracc]

Let's discuss briefly on Thu?

[sbenthall]

messages are not rendering properly, showing {0.url} etc. for some reason

[sbenthall]

Ok, string formatting is back in.

You can see the printed messages from the automated test with the -s flag on pytest i.e.

pytest -s tests/test_IndShockConsumerType.py