This repository has been archived by the owner on Apr 2, 2020. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
First level residuals #410
First level residuals #410
Changes from 32 commits
2b7df92
0663e7a
1b58ebb
4909feb
acac09b
4e7c897
0b15ec6
5f260c9
adb504c
9b434bc
d416d0b
65010cf
0b5a170
740f3bb
3d0ed81
5b61be4
bf319c1
08acd3a
fcf8320
a864919
953d15b
806f11d
51002ea
20e2201
a47586f
cdf2161
b8c483d
72aec1d
a245635
afef0a9
1cbd072
1a8e0c5
e801f8e
bf67066
b55c13b
999caf9
4b5d485
78995d0
44b0e85
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Private methods and functions should not have default args. Only public ones should. It's best to explicitly supply the arg for each parameter of private functions and methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
attribute is defined within a fixed dictionary of values I guess ? This should be provided if the function is meant to be used publicly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this should really be a private method (I've changed it to be a private method), but I added additional documentation and an internal check to be explicit anyway.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bthirion We should deprecate this and replace it with residuals in a separate PR, and others like it.
Or I should ask JD Kent to revert the changes I asked him to make and rename the resid mehotd in this PR back to
resid
so it stay consistent.I like the former idea, renaming all uses of
resid
toresiduals
.Does Nilearn have
resid
orresiduals
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, let's do it in a sperate PR.
I'm not aware of
resid
in nilear? We should go forresiduals
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think Nilearn has any resid method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so we will keep the method as
resid
for this pull request, I will not change thisThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bthirion @jdkent @Gilles86
I'm still confused about the suitability of this, and other such methods. It feels like
logL()
,resid()
andnorm_resid()
are in effect, traps; waiting for unsuspecting users to call them and watch their programs fail.If these methods do not have anything to say, then we should simply remove all of them, or convert this into a message rather than an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the motivation for this was to provide a hint to the user on how to get residuals from their model instead of an attribute error, but I see this check is handled here. I'm fine with removing those methods.