Add robustica method (internal PR review)

[Lestropie]

@BahmanTahayori:

There's a few things going on here. I've reverted a bunch of changes that were made, but rather than walking through the reversions I'll focus on what is.

The main branch on this fork has been reverted such that it no longer includes any commits made by yourself. Its tip currently points at 966a262.

New branch, add_robustica, contains only one commit, f4eaa3e. That one commit contains the sum total of all of your intended code changes. You can hopefully see from GitHub that that commit is authored by yourself, but committed by me; this is entirely faithful to the process that has happened, but in terms of software contribution tracking statistics it goes entirely to you.

This PR shows the proposed differences involved in merging the add_robustica branch from your fork into the main branch of your fork.

The plan is as follows:

1. We together make further changes to the add_robustica branch on your fork. These can be commits by either of us, based on discussion from either of us (or indeed others).
2. We close this PR without merging.
   Note that I'm explicitly making this a draft Pull Request to reflect the fact that it's not intended for merging.
3. We open a new PR, over at https://github.com/ME-ICA/tedana, where we propose merging the contents of the add_robustica branch on your fork into the main branch of the ME-ICA central version of TEDANA.

---

• Remove trailing whitespace
  (the GitHub diff viewer doesn't show these, but if you run git show HEAD on the current add_robustica branch, it will highlight such lines; I prefer to configure my code editor to delete these lines upon file save)
• Remove code comments that are personal notes
• I'll make a bunch of other comments that I'll attach to the code rather than writing here; bear in mind these will be based on looking at the diff and my experience with other software, not on knowledge of the TEDANA code base.

[Lestropie]

This is something I'd leave up to the TEDANA maintainers, but I'll lay the seed of the idea with you.
There's a large number of function keyword arguments here, and many look to be incompatible with one another. What might be a better long term solution would be to have a class that is responsible for storing an ICA configuration with respect to method, seeding, repeats, etc.. That class would have interface functions that would ensure that the configuration is always valid. An instance of that class could then be passed around the code.

[BahmanTahayori]

I agree with you and as you said we should leave it to the tedana team.

[Lestropie]

I presume the intention here is that if robust ICA is used, variable "fixed_seed" is meaningless. However the value "-99999" is not necessarily meaningless; it's an entirely meaningful integer value, and it's entirely possible that it could be used to seed an RNG depending on the API. It is more Pythonic to set the value of something that should not be used to None.

[BahmanTahayori]

This issue is fixed now as robustica is updated so that it achieves reproducible result with a seed.

[Lestropie]

• Should post hyperlink to robustica issue describing resolution of the issue.
• I think here you're saying "the issue" in reference to the failure of robustica to honour a fixed seed. However the context of this specific conversation is inclusion of the "fixed_seed=-99999" line. So the response should better communicate what has changed in response to that comment.

[Lestropie]

Delete

[BahmanTahayori]

The space has been removed.

[Lestropie]

This comment was targeting the commented line of code. While this happens all the time when evaluating possible code, such should not appear once you get to the point of proposing upstream changes.

[Lestropie]

• Unit testing: I noticed when going through their code base that they had some reasonably simple tests that process data with different numbers of echoes. It may be necessary to modify these tests. For instance:

  • For testing processing data with different numbers of echoes, ensure that fastica rather than robustica is used.
  • Add a new test that ensures that robustica works; perhaps with a smaller number of runs.

• The fact that it is being proposed that robustica become the default is quite consequential.

  • At the very least, it would be necessary to make some modification to the documentation to address the fact that, for the first software version including this change, execution time will be drastically longer.
  • The wider software maintenance team will want evidence that that increase in processing time is worthwhile. Note that this is not necessarily the same as the primary evidence generated for the publication: while that may be our ongoing recommendation, it is predicated on other modifications to one's processing pipeline, and the tedana team will only be interested in what is the most sensible default behaviour for the majority of their existing users. So they will want to see similar data but where the only change is the introduction of robustica, absent any PCA alterations.

[Lestropie]

• Add robustica to list of dependencies for miniconda environment suggested in README.md
  (These should all perhaps be a requirements.txt file?)

[BahmanTahayori]

Thanks for all your comments @Lestropie. Very helpful.

I have also added a test_integrity_robustica.py to the tests folder for testing robustica method.

When running the tests provided by the tedena team, there is one failure which is caused by naming. It is not from our changes and I think they are fixing it.

Please let me know if you have further comments. Thank you very much in advance.

[Lestropie]

GitHub shows a red circled dash at the end of some files. This indicates that the file does not contain a newline at the end. The final non-empty line of the file should contain a newline, but there should be no empty lines beyond that.

[Lestropie]

Fix space in file path.

[Lestropie]

Throughout much of the subsequent code, do not use "/" / "../" for generating filesystem paths. Use os.path.join(), os.pardir and os.path.normpath().

[Lestropie]

Are these data identical to those used in existing TEDANA tests for evaluating with use of FastICA? If so, it would be preferable if executing either FastICA or RobustICA tests would download the test data, but executing both tests would download those data only once.

[Lestropie]

Addition: Even if this turns out to be the current behaviour based on the subsequent code, it is also the case that the code responsible for doing so should appear only once, rather than being duplicated across the FastICA and RobustICA tests.

[Lestropie]

Spelling

[Lestropie]

I'm guessing that this is a duplication of existing TEDANA code, in which case the greater fault is said duplication rather than the content of the code itself. But worth mentioning anyway since I'm doing a code review and I see code.

This snippet introduces a race condition whereby if some other process deletes that directory in between the existence test and the deletion, the command will terminate. Better practise in Python is to do:

try:
    shutil.rmtree(out_dir)
except FileNotFoundError:
    pass

Or:

shutil.rmtree(out_dir, ignore_errors=True)

[Lestropie]

This is one of possibly multiple examples where it seems to me that the RobustICA-relevant file includes code that is not specific to RobustICA. It might be that I don't fully see the code path of how the tests are done. But I would expect that in the addition of RobustICA, there would be addition of tests to evaluate exclusively the new RobustICA features, rather than a duplicate "test everything" that includes things not relevant to RobustICA.

[Lestropie]

This is a bit too terse to be useful. If intent is something like "This applies to both fastica and robustica", say so more verbosely.

[Lestropie]

This magic number also appears in multiple locations, so should be defined as a const.

[Lestropie]

I'll need to do some more refactoring.

• f2cdb4e adds files in .idea/ that shouldn't be there; while it's possible to add new commits that erase them, it would be preferable for them to not appear in the history at all.
• ac4d008 adds merge conflict flags that shouldn't appear in the history.
• 41354cb needs a better commit message.

[BahmanTahayori]

Thanks for your comments, @Lestropie . Will consider the above points and will get back to you. Thanks.

[Lestropie]

I know I mentioned that there should be some kind of reference to the resolution of the fixed seed in robustica issue, but this doesn't work. If you look at the code naive to this history, it's a very odd code comment that seemingly bears no relevance to the surrounding code.

What I was advocating was something like, for instance, if it was the case that you perhaps made some code change where previously you were forbidding use of a fixed seed with robustica but it was now permitted, perhaps with an increment to the requisite robustica version number, then the commit making that change should include in its commit message a description of and hyperlink to that upstream change.

[Lestropie]

One thing of note here is that in cases where I can re-use your commits in some way, and therefore the original authorship is retained, the author description is "Circle tahayori@gmail.com". It would be better to set git up on your local system so that your commits are attributed to your name just as they are to your email.

[Lestropie]

Superseded by #3.