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)

[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]

LMK when this is ready for review.

[denisfouchard]

I think it is. We just have to figure out what is going on with experiments and disappointing results but appart from that code holds on

[emdupre]

Sorry, catching up on this conversation !

: 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 ?

Just to confirm, then, as I haven't seen that package : are we OK to copy this in re: its licensing ?

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

I'll look at the code additions to confirm, as "hyperalignment" often means different things to different people. If it's the 2016 Guntupalli implementation, though, I'd say it fits under template alignment, although more iterative in its estimation.

[denisfouchard]

Hey Elizabeth ! We have decided not to include the package as a dependency but to add it in a separate folder with the minimum code we need for the experiments.

It is true that it is by construction a template alignment method, the problem is that the way of computing templates is so different from other methods, that generally use simple tools such as Ridge or Procrustes, that adding Feeling's method in the TemplateAlignment would mean to double the size of the code, and lower the clarity of the code, that is why I think the best solution is to keep a minimal implementation on the side. Let me know what you think !

[emdupre]

Thanks, @denisfouchard !

Hey Elizabeth ! We have decided not to include the package as a dependency but to add it in a separate folder with the minimum code we need for the experiments.

Sorry for being unclear : What I mean is, did you write the code included in this PR in the hyperalignment folder, or was it taken from the hyperalignment package ? If the latter, we should confirm licensing !

[denisfouchard]

@emdupre The code I added is mainly mine, as well as the hyperalignment package I was importing before. Basically with @bthirion we took the unachieved and hard to understand codebase and reformated it and reimplemented some parts. So I think no worries with licensing, I cited the paper. @bthirion advised me to remove the name of the original researcher this model was coming from

[denisfouchard]

Sorry for the misunderstanding!

[emdupre]

Got it, thanks !

If I'm not mistaken, the code is actually contributed by us on a branch of the package that is unreleased. I don't think that it is subject to constraints, then ? The alternative is to recode the functions (there is room for improvement).

Any code that you've written yourselves is yours to relicense as many times as you see fit, released or unreleased 😸 Anything that you didn't write would be under the hyperalignment package license ; which might be compatible with ours, I just wanted to check ! But it sounds like this code is all written by you @bthirion and @denisfouchard, so it should be ok 👍

[denisfouchard]

Great !

I was rethinking about the integration. Maybe I can try to force integrate the method in the TemplateAlignment.py file, but I don't know if it would be a good idea, as Feilong's INT would stand out, with its own package and its own files to do everything ?

[bthirion]

Please keep it in an ad-hoc module.

[bthirion]

LMK when you need another round of reviews.

[emdupre]

👋 Hi @denisfouchard, and thanks for your work on this !

Let me know if it's useful for me to review now ; it looks like you and @bthirion are already going back and forth, but I'm happy to provide another set of eyes when you're ready.

[denisfouchard]

Hi @emdupre ! Appart from some merging issues that would arise from conflicts in versions of Nilearn calls (the changes you made did not come to my branch logically so I am still using an older version of Nilearn), I think the core code is ready to ship !

[denisfouchard]

If it is also good for @bthirion

[denisfouchard]

I have done my best trying to adress your comments, please let me know what you think and if you have remarks !

[bthirion]

As you can see, the documentation does not build properly. Can you check what's happening ? Thx in advance

[denisfouchard]

I again tried to address as many remarks as I could, especially doc inconsistencies and int_plot issues. I have still to address the srm_plot, which I will do as soon as I can !

[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]

@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 ?

[bthirion]

Merging, then. Thx to all !

[denisfouchard]

Great ! Thank you for the reviews !