Add shape check to Dataset initialization #106
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #106 +/- ##
==========================================
+ Coverage 87.60% 87.88% +0.28%
==========================================
Files 13 13
Lines 863 883 +20
==========================================
+ Hits 756 776 +20
Misses 107 107 ☔ View full report in Codecov by Sentry. |
|
Oh BTW you should add yourself to the Zenodo file. I totally forgot about that. |
| # Raise error if the number of rows and columns of v don't match y | ||
| with pytest.raises(ValueError): | ||
| utils._check_inputs_shape(y, v, "y", "v", row=True, column=True) | ||
|
|
||
| # Raise error if neither row or column is True | ||
| with pytest.raises(ValueError): | ||
| utils._check_inputs_shape(y, n, "y", "n") |
There was a problem hiding this comment.
Sorry, I know I approved already, but I just realized that, while the function allows Nones, that behavior isn't tested here. Can you test Nones? Not every Dataset will have v or n.
That's a great idea. I think that can be implemented by checking the shape looping through a list of axis given by the user: utils._check_inputs_shape(y, X, "y", "X", axis=[0])
utils._check_inputs_shape(y, n, "y", "n", axis=[0, 1])
utils._check_inputs_shape(X, np.array(X_names)[None, :], "X", "X_names", axis=[1]) |
pymare/utils.py
Outdated
| elif (param1 is None) or (param2 is None): | ||
| # If param1 or param2 is None, we don't need to check the shape | ||
| pass |
There was a problem hiding this comment.
No, I don't think we need that.
There was a problem hiding this comment.
If you're okay dropping this clause, then, I'll be happy to approve.
There was a problem hiding this comment.
Thanks! Would you like to add support for N-dimensional arrays in this PR? I think I have got it working.
There was a problem hiding this comment.
I don't think we need N-dim support yet. I'm happy to merge as-is.
EDIT: Once the extra clause is removed, I mean.
Closes #99.
Changes proposed in this pull request:
_check_inputs_shape()function toutils.pyto avoid repetitive code.ymatchesX.ymatchv.ymatchn.Note: I didn't check for the number of columns of
Xvs the length ofX_names, because an exception is raised in case of any mismatch when_get_predictors()is applied.