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

Adds 2D response matrix check #90

Merged
merged 3 commits into from Aug 30, 2018
Merged

Adds 2D response matrix check #90

merged 3 commits into from Aug 30, 2018

Conversation

jrbourbeau
Copy link
Owner

@jrbourbeau jrbourbeau commented Aug 27, 2018

This PR adds a check that input response matrices are 2-dimensional. Now a more meaningful error message will be raised when a non-2D response matrix is used.

Ref: #89

  • Tests added / passed
  • Passes flake8 pyunfold

@zhampel
Copy link
Collaborator

zhampel commented Aug 28, 2018

Hey @jrbourbeau, in the new test test_mixer_2d_response_check() in test_mix.py that you've added, it declares a new response matrix, but I see at the top of that script that this has arguments num_effects, num_causes, num_effects. Can you please explain why the extra num_effects is needed? Ideally, there is one set of effects and one can add any number of causes to the response matrix.

@jrbourbeau
Copy link
Owner Author

That test is checking that an error is raised if the input response matrix is not 2-dimensional. The extra num_effects is just to specify a certain size for the third dimension of the (bad) response matrix. Instead of

response = 1 + np.random.rand(num_effects, num_causes, num_effects)

I can add a comment and switch to something like

# Construct a response matrix that is 3-dimensional (should raise an error)
response_bad = 1 + np.random.rand(num_effects, num_causes, 1)

to make it more clear what the motive is for the test.

@zhampel
Copy link
Collaborator

zhampel commented Aug 29, 2018

@jrbourbeau Oh yes of course! Jet lag is killer, I wasn't thinking straight but indeed, you need more dims for this check! :) Thanks

Looks good to me especially with the renaming suffix '_bad'. Passes inspection 👍

Copy link
Collaborator

@zhampel zhampel left a comment

Choose a reason for hiding this comment

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

Thanks for adding the '_bad' suffix for clarity. Now it makes uber sense what the test is doing and testing. Approval 👍

@jrbourbeau jrbourbeau merged commit bf1ce87 into master Aug 30, 2018
@jrbourbeau jrbourbeau deleted the check_response_2d branch August 30, 2018 01:50
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.

None yet

2 participants