jdkent commented 4 years ago

Continuation of PR #298

codecov[bot] commented 4 years ago

Codecov Report

Merging #410 into master will increase coverage by 0.03%. The diff coverage is 94.66%.

@@            Coverage Diff             @@
##           master     #410      +/-   ##
==========================================
+ Coverage   92.66%   92.69%   +0.03%     
==========================================
  Files          40       40              
  Lines        4514     4588      +74     
==========================================
+ Hits         4183     4253      +70     
- Misses        331      335       +4

Impacted Files	Coverage Δ
nistats/regression.py	`77.31% <100%> (+0.72%)`	:arrow_up:
nistats/tests/test_first_level_model.py	`100% <100%> (ø)`	:arrow_up:
nistats/tests/test_regression.py	`100% <100%> (ø)`	:arrow_up:
nistats/first_level_model.py	`83.12% <85.18%> (+0.18%)`	:arrow_up:
nistats/tests/test_reporting.py	`96.82% <0%> (ø)`	:arrow_up:
nistats/tests/test_glm_reporter.py	`97.93% <0%> (ø)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 755039f...44b0e85. Read the comment docs.

jdkent commented 4 years ago

I believe this is ready for review, @Gilles86, could you take a look to make sure I didn't fundamentally alter your pull request?

Gilles86 commented 4 years ago

Very cool. Will try to have a look this week.

kchawla-pi commented 4 years ago

Hi @jdkent thanks for looking after this Feature.

kchawla-pi commented 4 years ago

@bthirion I know we avoid using @property when we can. The functions residuals & predictions will really serve better as properties instead of methods.

bthirion commented 4 years ago

@bthirion I know we avoid using @property when we can. The functions residuals & predictions will really serve better as properties instead of methods.

Which is why the setattr_on_read decorator is used. I think that this is OK.

jdkent commented 4 years ago

Except for the codecov patch error, I think this is ready for review. I personally find patch coverage to be a rather noisy measure of a pull request, I have a very lenient setting in one of my own projects. But if the patch diff is important to fix, I can add a couple new tests so that "more lines" are covered for this pull request.

kchawla-pi commented 4 years ago

Which is why the setattr_on_read decorator is used. I think that this is OK.

Why is that preferable to a property in this case? A property can be repeatedly used as an an attribute, simplifying access for users. setattr_on_read makes the method usable as an attribute only once, & if that's the case then we don't need decorator at all. This will create confusion for users, if they access the method as an attribute once & then watch it fail the second time. If I am missing something here, explain it to me. If not, we should make these properties.

kchawla-pi commented 4 years ago

@bthirion best we wrap this one up before holidays. @jeromedockes What's your opinion about the setattr_on_read and properties discussion we're having here

jeromedockes commented 4 years ago

@jeromedockes What's your opinion about the setattr_on_read and properties discussion we're having here

No strong opinion. using setattr_on_read is fine. it is a caching mechanism so it could also be an option to use the firstlevelobject's joblib memory for this. it would also be perfectly fine (perhaps best) to just have a get_residuals() function and let the user hold a reference to the result if they want.

is this going to be showcased in an example?

kchawla-pi commented 4 years ago

Okay setattr_on_read is fine then.

It turns out that FirstLevelModel standardizes the data across time by default. I have to say I personally don't like that, because if you don't know this, it's very hard to understand to know what's going on... Any thoughts about this?

That would be beyond the purview of this PR I think. You should file an issue, and we can look into that before or after the freeze.

kchawla-pi commented 4 years ago

This seems nearly ready. @bthirion any further comments? Anything about @Gilles86 FLM time normalized comment? @jdkent Attend to the missing docstrings and too may blank lines, and if there are no more objections, we can merge.

jeromedockes commented 4 years ago

This seems nearly ready.

still thinking about the upcoming merge into nilearn: if this were nilearn we would probably ask for this to be shown in an example

Gilles86 commented 4 years ago

I can have a go at it.

On Wed, Dec 18, 2019 at 11:20 AM jeromedockes notifications@github.com wrote:

This seems nearly ready.

still thinking about the upcoming merge into nilearn: if this were nilearn we would probably ask for this to be shown in an example

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nistats/nistats/pull/410?email_source=notifications&email_token=AAIPRTRJ43S3WT4BLNYUYIDQZH2QNA5CNFSM4JPMYOJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHFUMOA#issuecomment-566969912, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIPRTSBFQKUYFXZYWBU3YDQZH2QNANCNFSM4JPMYOJA .

-- Gilles de Hollander Postdoctoral Researcher University of Zurich Department of Economics website http://www.gillesdehollander.nl/ | github https://github.com/Gilles86/

jeromedockes commented 4 years ago

also making that error message : https://github.com/nistats/nistats/pull/410/files#r359257721 a bit more verbose would be great -- if that is confusing for developers it certainly will be for users. but I can also do it in another PR if necessary

Gilles86 commented 4 years ago

https://github.com/jdkent/nistats/pull/1

kchawla-pi commented 4 years ago

Pull from master to fix the CircleCI failure.

kchawla-pi commented 4 years ago

Nevermind, my comments about same docstring are invalid.

kchawla-pi commented 4 years ago

@bthirion Upto you now to review the example.

kchawla-pi commented 4 years ago

@bthirion As discussed, we REALLY need to review and merge this ASAP! @jdkent I was going to resolve the conflict, update the master, and convert nosetest calls to pytest, so you don't have to, but seems you have disabled maintainer access to your branch? If so, you will either have to make changes yourself (they are not much) or I can open a PR for you to accept.

kchawla-pi commented 4 years ago

This seems ready to merge. Anything else @bthirion ?

kchawla-pi commented 4 years ago

@jdkent Add whats_new entry and also update your name in the .mailmap .

kchawla-pi commented 4 years ago

Phenomenal work @jdkent and @Gilles86 ! Thanks to both of you!!

Gilles86 commented 4 years ago

Thank you!

Nistats is one of my favorite python neuroimaging packages. Keep up the good work!

nilearn / nistats

First level residuals #410

Codecov Report