Functionalize out each condition from checkConditions

[sbenthall]

See #534

In order to save on code duplication, I've functionalized out each condition from checkConditions.

[MridulS]

this needs to be there @sbenthall as the variable url is referenced multiple times.

[MridulS]

ah wait I see this here, other functions like checkGICInd also use the var url can you make it self.url so that references can be passed?

[llorracc]

@sbenthall, this looks great (but don't have time to review it now).

Key test will be whether it works as-is with BufferStockTheory, or whether BST needs to be revised (I'm guessing the latter).

[sbenthall]

Tests pass with the current changes.
Any functionality that broke didn't have tests, but no change in functionality was intentional.

@MridulS I can fix that url variable issue tomorrow. thanks for catching it.

[MridulS]

This also breaks DemARKs. Seb can you run locally all the DemARKs? As even on travis we run a subset due to time constraints. We should probably test against DemARKs whether we make changes to the core codebase. They are as close to tests we have.

…

On Sat, 22 Feb 2020 at 04:11, Sebastian Benthall ***@***.***> wrote: Tests pass with the current changes. Any functionality that broke didn't have tests, but no change in functionality was intentional. @MridulS <https://github.com/MridulS> I can fix that url variable issue tomorrow. thanks for catching it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#536?email_source=notifications&email_token=ABI5RFCRSK7IRDYAEZA2QF3RECJXBA5CNFSM4KZGKEHKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMUVMHQ#issuecomment-589911582>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABI5RFAYZNYCLJZXMSPQ2HLRECJXBANCNFSM4KZGKEHA> .

[sbenthall]

Any way you could link me to the Travis run of the DemARKs so I can see the errors there?

[sbenthall]

Ok, @MridulS I've ran the DemARKs locally. That was a good idea.

#540 has fixes for the bugs that came up that seemed related to this PR.
I filed some other issues to the DemARK issue tracker for other things that came up.

In general:

• I would much rather have pull request reviews before merging. I rushed this one because of the BST timing. I think it's always smoother if we plan these things out ahead of time.
• Thanks @MridulS for catching the errors.
• I think it's a problem if the only "test suite" we have takes so long to run that we can't even have Travis do it, so we have to run it locally instead. That's backwards!
• I don't like the smell of the current code on this, and I think that the thing to do is to do a further refactor, with granular, fast tests in the HARK test suite, slated for the release following the immediate, urgent one.

[MridulS]

• I would much rather have pull request reviews before merging. I rushed this one because of the BST timing. I think it's always smoother if we plan these things out ahead of time.
• I think it's a problem if the only "test suite" we have takes so long to run that we can't even have Travis do it, so we have to run it locally instead. That's backwards!

Yeah, I caught this because I keep a check on DemARKs with master HARK, we could add DemARK check before merging in anything to HARK master but as it takes some time to run the DemARK travis build we decided against it. Travis will happily run it but it just takes some time. You can run the DemARKs locally to test against changes to your local branch before issuing a PR by following this workflow

# install the updated HARK version in the current environment
$ pip install -e . 
# change to DemARK repo
$ cd path_to_demark
# run pytest
$ pytest --nbval-lax notebooks/

Especially when we are touching the model codebase we should make sure this one works out. This may not be the best way to getting this done but until we have proper unit testing framework this should be the way to go.

[llorracc]

I think in our prior discussion of this we concluded that the best workflow was exactly what happened in this case: Travis wouldn't hold things up for a complete test of everything (including DemARKs) but @mridul Seth <[email protected]> would set up a nightly test that could take a long time but would catch more bugs, by (e.g.) testing DemARKs. So, the system worked!

…

On Mon, Feb 24, 2020 at 1:17 PM Mridul Seth ***@***.***> wrote: - I would much rather have pull request reviews before merging. I rushed this one because of the BST timing. I think it's always smoother if we plan these things out ahead of time. - I think it's a problem if the only "test suite" we have takes so long to run that we can't even have Travis do it, so we have to run it locally instead. That's backwards! Yeah, I caught this because I keep a check on DemARKs with master HARK, we could add DemARK check before merging in anything to HARK master but as it takes some time to run the DemARK travis build we decided against it. Travis will happily run it but it just takes some time. You can run the DemARKs locally to test against changes to your local branch before issuing a PR by following this workflow # install the updated HARK version in the current environment $ pip install -e . # change to DemARK repo $ cd path_to_demark # run pytest $ pytest --nbval-lax notebooks/ Especially when we are touching the model codebase we should make sure this one works out. This may not be the best way to getting this done but until we have proper unit testing framework this should be the way to go. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#536?email_source=notifications&email_token=AAKCK72SH5PXNW3D3BOKZU3REO3GZA5CNFSM4KZGKEHKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMXSSYA#issuecomment-590293344>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKCK75TZCCODT7RSLOBFLTREO3GZANCNFSM4KZGKEHA> .

-- - Chris Carroll

[sbenthall]

Hmm. Ok.

@MridulS I didn't realize that pytest could work with notebooks in that way. Cool!
Is this workflow documented anywhere?

I think that in any case, it would be better if there were more automated tests in HARK that covered the functionality in DemARKs without having as long a runtime.
I've made a ticket for this here:

#542