-
Notifications
You must be signed in to change notification settings - Fork 55
Conversation
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.
LGTM
…me-resid * 'master' of https://github.com/nistats/nistats: First level residuals (nilearn#410) Replace numpy's deprecated dec with pytest's skipif (nilearn#423)
Codecov Report
@@ Coverage Diff @@
## master #425 +/- ##
==========================================
+ Coverage 92.58% 92.65% +0.06%
==========================================
Files 40 40
Lines 4694 4708 +14
==========================================
+ Hits 4346 4362 +16
+ Misses 348 346 -2
Continue to review full report at Codecov.
|
Should I rename the method |
On 13/01/2020 17:05, Kshitij Chawla wrote:
Should I rename the method |norm_resid|, and parameter |df_resid|?
What about the options, |['resid', 'norem_resid',...]| ?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#425>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABZHVTWLEXRP3W67BFDJ3TQ5SGODANCNFSM4KFKDFEA>.
I think so.
But do we really need all these parameters ? (see #210, that I need to
revive bzw).
|
@bthirion If you are happy with the name changes, I will add tests for the changes. If there's an attribute/method you don't want changed tell me now and I won't add a test for that. Also, some attributes might require to be treated as If you think we should just change the names and no think about backwards compatibility, since this is still technically beta, let me know that as well. |
The deprecated attributes might need to be set as property, not the final ones. |
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.
LGTM
Actually, I'd like to remove wresiduals and normalized residuals asap. But say, for another PR.
If you want to and are able to quickly work on that PR, while I adapt the GLM Reporter for multidimensional contrasts, that's fine with me. As long as your PR doesn't take too long. |
…me-resid * 'master' of https://github.com/nistats/nistats: Design matrices may have different columns (nilearn#416) Improve reporting: Correct alpha default value & enable 2 sided map_threshold (nilearn#418) Prevent duplication & insertion of reports from damaging website layout (nilearn#429) Fix whatsnew and the website's News/updates section (nilearn#428) GLM Reports: Computed masks are used if there is no user supplied one (nilearn#424)
- Reduced verbosity in warning message. - Renamed wresiduals to whitened_residuals.
@bthirion Another review. |
remove it --- if possible.
… De: "Devon Hjelm" ***@***.***>
À: "nistats" ***@***.***>
Cc: "Bertrand Thirion" ***@***.***>, "Mention"
***@***.***>
Envoyé: Mardi 21 Janvier 2020 17:31:58
Objet: Re: [nistats/nistats] Renamed resid methods to resdiuals (#425)
[ https://github.com/bthirion | @bthirion ] Another review.
What do you wanna do with wY ?
—
You are receiving this because you were mentioned.
Reply to this email directly, [
#425
| view it on GitHub ] , or [
https://github.com/notifications/unsubscribe-auth/AABZHVV7D7FZCJGJZ32KAQTQ64PP5ANCNFSM4KFKDFEA
| unsubscribe ] .
|
Remove it? Or rename it to |
Sorry, in this PR we should rename it to 'Y_whitened', but in the next PR, I'd love to remove it. |
…me-resid * 'master' of https://github.com/nistats/nistats: Replace packages installed from conda-forge by PyPI (nilearn#431)
I'm fine with the changes. Is it really meaningful to have deprection warning, since we're going to freeze nistats very soon ? |
…ate-flake8 * 'master' of https://github.com/nistats/nistats: Renamed resid methods to resdiuals, w to whitened_ (nilearn#425) # Conflicts: # nistats/model.py
No description provided.