Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding testing and validation #188

Merged
merged 8 commits into from
Apr 21, 2020
Merged

Adding testing and validation #188

merged 8 commits into from
Apr 21, 2020

Conversation

arthir
Copy link
Contributor

@arthir arthir commented Apr 17, 2020

Various changes:

  • Moving experiment config yamls into their own folder
  • Added unit tests for function which processes the parameter function description
  • Modified the date->timestep processing to work off an existing column

@arthir arthir requested a review from stephen-hoover April 20, 2020 20:13
@arthir
Copy link
Contributor Author

arthir commented Apr 20, 2020

@stephen-hoover I added some basic tests for the parsing (and this also sets up a basic testing framework).

Copy link
Collaborator

@stephen-hoover stephen-hoover left a comment

Choose a reason for hiding this comment

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

This all LGTM!

Some comments on things related to this PR that I've found useful:

  • There's a pd.testing.assert_frame_equal function which is great for testing changes to DataFrames. Similarly, pd.testing.assert_series_equal.
  • I find set literals easier to read than function calls, set([10]) == {10}.
  • For testing with random numbers, I'd suggest creating a np.random.Generator object with controlled seed in the test function. That would require modifying add_config_parameter_column to accept an optional Generator parameter, which seems out of scope for this PR.

@arthir
Copy link
Contributor Author

arthir commented Apr 20, 2020

  • There's a pd.testing.assert_frame_equal function which is great for testing changes to DataFrames. Similarly, pd.testing.assert_series_equal.
    Of course. I should have thought of this instead of the long winded way I was testing it.
  • I find set literals easier to read than function calls, set([10]) == {10}.
    Makes sense - I think I removed all of these anyways.
  • For testing with random numbers, I'd suggest creating a np.random.Generator object with controlled seed in the test function. That would require modifying add_config_parameter_column to accept an optional Generator parameter, which seems out of scope for this PR.
    Ah, that's a good point but yes, I think out of scope for now.

@arthir arthir requested a review from stephen-hoover April 20, 2020 22:17
Copy link
Collaborator

@stephen-hoover stephen-hoover left a comment

Choose a reason for hiding this comment

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

LGTM!



def test_add_config_parameter_column__error():
f = {'weird_function': {}}
with pytest.raises(ValueError) as e:
with pytest.raises(ValueError, match="Unknown type of parameter"):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Neat! I didn't know about the match kwarg. That's useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Josh told me about this one today -- it's so nice!

@arthir arthir marked this pull request as ready for review April 21, 2020 02:08
@arthir arthir requested a review from ManuelaRunge April 21, 2020 02:09
@ManuelaRunge ManuelaRunge merged commit e28f752 into numalariamodeling:master Apr 21, 2020
jacksonllee pushed a commit that referenced this pull request Feb 10, 2021
* DOC/TST Added documentation, moved configs, added (some) tests

* Adding dev-requirements

* datetotimestep is computing based on another column, instead of a constant

* cleaning up

* Fixing merge

* TST making fixes based on tests

* TST cleaning up test cases
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.

3 participants