Add Parcel and Searchlight Hyperalignment methods

[denisfouchard]

For future reference, this PR is intended to add the Individualized Tuning model (see paper) into fmralign and compare it to FastSRM (H. Richard et. al. , 2019)

[bthirion]

Are these things tested or demoed somewhere ?

[emdupre]

Hi @denisfouchard,

Thanks for this addition ! Just to +1 @bthirion 's comments, it'd be great to have these new calls explicitly added into new examples and tests. This is adding a new dependency to the library, so it'd be especially useful to showcase why it's a good addition.

Just to note : you'll also need to modify the pyproject.toml accordingly ! The test errors are associated with recent nilearn updates, so it's hiding what will later be an import error for the hyperalignment package.

In the meantime, I'll update these nilearn calls so your changes are against a passing main branch !

[denisfouchard]

Hi @emdupre and thanks for the message ! I will review and adapt to your suggestions in the days to come

In the meantime, enjoy your end of the year festivities !

[denisfouchard]

Hi @bthirion I will add plots to the package. For the tests, there are in the hyperalignment package I have been working on from the begining ;)

[denisfouchard]

There is an other problem, that we will need to adress at some point (maybe at the end of holidays) : we are importing the Hyperalignment package that is going to be private for the rest of its life maybe (as long as Feilong keeps it private at least), so we will need maybe to adapt the code ? We can not just add hyperalignment to the requirements and pip install it directly

[bthirion]

We don't want to depend on hyperalignment. We need to copy the relevant code here.

[denisfouchard]

We don't want to depend on hyperalignment. We need to copy the relevant code here.

The problem is that there is a lot of code, and a lot of files. It would imply to copy a ton of files in the package

[bthirion]

Yes, but not much more than other estimators. The point is to factorize as much as possible.

[denisfouchard]

I'll do my best.
But for example FastSRM is imported, not integrated, and hyperalignment uses a lot of very custom code

[bthirion]

FastSRM has been worked out quite well, and is relatively high quality, this is why we can rely on it.

[denisfouchard]

I tried to compress and remove all the unnecessary stuff to include it directly in fmralign. Sadly I think I can not go further, as hyperalignment does not seem to fit in the "template_alignment". It is its own unique way of doing alignment

[denisfouchard]

If we really want to have the minimum amount of files I can put everything in one file, but this is a bit risky and not very pleasing to read.

[bthirion]

Looks great. I think there is room for improvement, by abiding better to sklearn's conventions.

[bthirion]

Indeed.
I'm assuming that you have enough tests and examples to control what you're doing.
Anyway, git is your friend, you can always revert changes.

[denisfouchard]

.

Should be ok now !

[bthirion]

I think it can be merged now.

[denisfouchard]

@emdupre what do u think ? :)

[emdupre]

Thank you, @denisfouchard !! This looks great 🚀 Only small comments, and then I think it's good to go !

[denisfouchard]

I have made changes according to @emdupre's latest comments. Tbh I don't want to adress now comments about code choices that were made at the begining and that were left uncommented for the whole 3 last months 😅. I did what I could, but I think it is the best we can do for INT !

[emdupre]

I have made changes according to @emdupre's latest comments. Tbh I don't want to adress now comments about code choices that were made at the begining and that were left uncommented for the whole 3 last months 😅. I did what I could, but I think it is the best we can do for INT !

Thanks, @denisfouchard ! I know that longer reviews are frustrating, but please remember that this is a huge amount of code (for everyone) with very little context (e.g. the conversations on licensing, notification of ready-to-review status, etc) for me.

Regardless, it's fine if you don't want to address at this point any potentially remaining code changes in this PR, but please at least respond to comments brought up in a review so we know if we need to open a dedicated issue.

Thanks again for your work on this.

[denisfouchard]

I have made changes according to @emdupre's latest comments. Tbh I don't want to adress now comments about code choices that were made at the begining and that were left uncommented for the whole 3 last months 😅. I did what I could, but I think it is the best we can do for INT !

Thanks, @denisfouchard ! I know that longer reviews are frustrating, but please remember that this is a huge amount of code (for everyone) with very little context (e.g. the conversations on licensing, notification of ready-to-review status, etc) for me.

Regardless, it's fine if you don't want to address at this point any potentially remaining code changes in this PR, but please at least respond to comments brought up in a review so we know if we need to open a dedicated issue.

Thanks again for your work on this.

Sure, I think I replied to every comment ! (I did move the functions of toy data generations as you proposed)

LMK if it is good for you or if anything should have its own PR if ;)

[bthirion]

@emdupre WDYT ?

[emdupre]

I think this is ready 🚀

Thanks @denisfouchard for the (huge) lift !! Excited to see this added.

[bthirion]

Merging, then. Thx to all !

[denisfouchard]

Great ! Thank you for the reviews !