FIX: Miscelaneous revisions to make tests execute properly #84
Conversation
|
Hello @oesteban! Thanks for opening this PR. We checked the lines you've touched for PEP 8 issues, and found:
|
There was a problem hiding this comment.
Great. I think that I understand this better now. In general, it seems that these model fit and predict methods should always take gradient table inputs (i.e. 4-by-n arrays). I note a place where this is not the case, and I wonder whether it should be enforced more strictly.
| @@ -240,7 +240,7 @@ def predict(self, gradient, **kwargs): | |||
| class AverageDWModel: | |||
| """A trivial model that returns an average map.""" | |||
|
|
|||
| __slots__ = ("_data", "_gtab", "_th_low", "_th_high", "_bias", "_stat") | |||
| __slots__ = ("_data", "_th_low", "_th_high", "_bias", "_stat") | |||
|
|
|||
| def __init__(self, gtab, **kwargs): | |||
There was a problem hiding this comment.
Do you want to also remove the gtab input here?
| def __init__(self, gtab, **kwargs): | |
| def __init__(self, **kwargs): |
There was a problem hiding this comment.
If yes, don't forget to remove it in the docstring as well.
| self._th_low = kwargs.get("th_low", 50) | ||
| self._th_high = kwargs.get("th_high", self._gtab[3, ...].max()) | ||
| self._th_high = kwargs.get("th_high", 10000) |
There was a problem hiding this comment.
I know this is just a toy model, but we might want to document the default values for these, in case they get inherited into other places.
| ) | ||
| tmodel_2000 = model.AverageDWModel( | ||
| gtab=gtab, bias=False, th_high=2000, th_low=1000 | ||
| gtab=gtab, bias=False, th_high=2000, th_low=900, stat="mean", |
There was a problem hiding this comment.
Ha! I just noticed that the default statistic for the "mean" model is the median. Isn't that a bit confusing?
|
|
||
| # Verify that the right statistics is applied and that the model discard b-values < 50 | ||
| assert np.all(tmodel_mean.predict() == np.mean(gtab_w25[:, :2], axis=0)) | ||
| assert np.all(tmodel_median.predict() == np.median(gtab_w25[:, :2], axis=0)) | ||
| assert np.all(tmodel_mean.predict([0, 0, 0]) == 950) |
There was a problem hiding this comment.
Isn't the input to predict supposed to be a gradient table? That is, a 4-by-n array? Why does it accept a 3-vector?
|
Thanks for the review, I haven't had time to come back to it, but will ASAP! Sorry for the slow response. Overall, all you said seems very sensible. |
|
I will go ahead and merge this, so that we can keep working on this on our end with functioning tests. We can come back and fix my comments on a separate PR. |
|
Thanks, I'm coming back to this tomorrow and finishing up whatever is still missing. Running tests locally right now after merge. |
Although this is not going to make all tests to pass, it at least ensures tests can be executed provided with test data fount under
$TEST_DATA_HOME.This PR also fixes a bug on the
AverageDWModelthat resulting in the indexing error.