Leverage seaborn in figure creation

[tsalo]

Summary

The classification plot code uses seaborn, but doesn't take advantage of the available features. We should review the plotting code and identify places where it can be improved.

Additional Detail

An example- instead of using seaborn's hue argument, the classification plot code duplicates the same lines for the accepted and rejected components. This also requires the code to add fake rows in cases where all components are either rejected or accepted.

Next Steps

1. Review the classification plot code for unnecessary duplication.
2. Fix up seaborn usage.

[tsalo]

Here is an example of duplication that can be eliminated by using the hue argument:

https://github.com/Brainhack-Donostia/ica-aroma-org/blob/10f5183671d2a5014b96bc10447ac6d41d9f792b/aroma/plotting.py#L144-L156

[eurunuela]

This is a great little task for anyone from Brainhack Donostia joining the project!

[oesteban]

This is a great little task for anyone from Brainhack Donostia joining the project!

Yes!, but I would say this should be contributed directly upstream to ICA-AROMA. I think this re-implementation should focus only on the core of the method.

I also know for experience that the plotting module of ICA-AROMA is not very stable, so you don't want to bring that here. Instead, you want all the ICA-AROMA current users to benefit from improvements making the plotting better.

WDYT?

[vinferrer]

Maybe we can implement here and if everything goes smoothly propose the changes in the original ICA-AROMA

[tsalo]

We can try. The original repo is rarely updated, which is why AROMA was such a good target for this kind of project. Still, once we have things working here, we can always offer to open a PR...

[vinferrer]

You meant something like this tsalo?

[vinferrer]

I can change the colours so they are the same as the others

[vinferrer]

Also there are a few deprecated messages

[tsalo]

The figure looks good, although I'd have the scatter follow the same palette as the rest of the elements.

It's hard to say if the update reflects what I opened this issue about without looking at the code though, since it's mostly a matter of refactoring rather than enhancing the outputs.

[vinferrer]

solve it

[vinferrer]

i am gonna see what can i do about this:

/usr/local/lib/python3.8/dist-packages/seaborn/distributions.py:2551: FutureWarning: `distplot` is a deprecated function and will be removed in a future version. Please adapt your code to use either `displot` (a figure-level function with similar flexibility) or `histplot` (an axes-level function for histograms).
  warnings.warn(msg, FutureWarning)
/usr/local/lib/python3.8/dist-packages/seaborn/distributions.py:2551: FutureWarning: `distplot` is a deprecated function and will be removed in a future version. Please adapt your code to use either `displot` (a figure-level function with similar flexibility) or `histplot` (an axes-level function for histograms).
  warnings.warn(msg, FutureWarning)
/usr/local/lib/python3.8/dist-packages/seaborn/distributions.py:2551: FutureWarning: `distplot` is a deprecated function and will be removed in a future version. Please adapt your code to use either `displot` (a figure-level function with similar flexibility) or `histplot` (an axes-level function for histograms).
  warnings.warn(msg, FutureWarning)
/usr/local/lib/python3.8/dist-packages/seaborn/distributions.py:1649: FutureWarning: The `vertical` parameter is deprecated and will be removed in a future version. Assign the data to the `y` variable instead.
  warnings.warn(msg, FutureWarning)
/usr/local/lib/python3.8/dist-packages/seaborn/distributions.py:2551: FutureWarning: `distplot` is a deprecated function and will be removed in a future version. Please adapt your code to use either `displot` (a figure-level function with similar flexibility) or `histplot` (an axes-level function for histograms).
  warnings.warn(msg, FutureWarning)
/usr/local/lib/python3.8/dist-packages/seaborn/distributions.py:1649: FutureWarning: The `vertical` parameter is deprecated and will be removed in a future version. Assign the data to the `y` variable instead.
  warnings.warn(msg, FutureWarning)

[tsalo]

Yes, distplot needs to be replaced with displot or histplot. I think either would probably be fine.

[oesteban]

Maybe we can implement here and if everything goes smoothly propose the changes in the original ICA-AROMA

I would not do this. You'll need to add matplotlib, which is a pretty beefy dependency (through seaborn). If you want to keep this project focused, I'd focus in making visualization a separate project (and if you love OSS, submit the changes to the aroma repo).

[tsalo]

You'll need to add matplotlib, which is a pretty beefy dependency (through seaborn).

What if we make it an optional dependency, set up a "plotting" dependencies subgroup, and change the argument --noplots to --plots so they aren't generated by default? That way, tools that don't want those heavy dependencies (e.g., fMRIPrep) won't need to install them, while users who want to use AROMA directly can.

(and if you love OSS, submit the changes to the aroma repo).

I mean... the ICA-AROMA repo hasn't accepted proposals to (1) refactor into a project or (2) deploy to pypi, even after >=2 years of requests. Both of which would have made this refactor mostly unnecessary (minus all of the FSL calls). The inability to get changes into the main repository is a major reason why https://github.com/nipreps/fmripost-aroma/issues/10 and https://github.com/nipreps/fmriprep/issues/1784 have stalled (at least on my end).

[oesteban]

Yup, but both nipreps/fmripost-aroma#10 and nipreps/fmriprep#1784 are better off with the leanest possible re-implementation, so dropping the plotting (or separating it) and dropping moving stuff to standard space are two appealing features.

From fMRIPrep perspective, we want to get to the core of aroma. For the plotting, I'm sure tedana actually has a lot of stuff that could be adaptable.

[vinferrer]

Okay,

Yes, distplot needs to be replaced with displot or histplot. I think either would probably be fine.

Actually we have a problem about this.

1. displot is a figure-level function we cannot use it to plot the distribution plots in the axes.
2. hisplotdoesn't have the vertical argument, which makes not possible to plot in vertical it seems. Result:

[tsalo]

@vinferrer It's probably best to just use a jointplot then. Honestly, even if displot and histplot could do what we wanted, a jointplot still would have been better.

[vinferrer]

@tsalo that's a nice suggestion but i think at this point jointplot does not work easily with the subplot setting we have, look at this https://stackoverflow.com/questions/35042255/how-to-plot-multiple-seaborn-jointplot-in-subplot.

[tsalo]

😞 so much for being a GFI. Honestly, with this much work going into it, maybe we should just take @oesteban's suggestion and drop plotting entirely. It doesn't tell us much (especially not compared to a component-wise breakdown like what fMRIPrep outputs), and it's too much effort to keep working.

[tsalo]

And I don't want to pin our dependencies to an old version of seaborn, since that might impact the rest of our dependencies.

[vinferrer]

Well, we could generate the image separately and then plot in the subplot a png of that separated image

[vinferrer]

let my give a last try

[vinferrer]

I almost have it, just need a bit of help on how to increase the first subplot size

[vinferrer]

I know it is related to these lines:

    # define grids
    gs = gridspec.GridSpec(4, 7, wspace=1)
    gs00 = gridspec.GridSpecFromSubplotSpec(4, 4, subplot_spec=gs[:, 0:3])
    gs01 = gridspec.GridSpecFromSubplotSpec(4, 1, subplot_spec=gs[:, 3:5])
    gs02 = gridspec.GridSpecFromSubplotSpec(4, 1, subplot_spec=gs[:, 5:7])

But i have never use this matplotlib configuration

[eurunuela]

This looks amazing! Thank you @vinferrer !

I almost have it, just need a bit of help on how to increase the first subplot size

Maybe using a tight layout helps with the size? I cannot think of any other way of making it bigger right now.

[vinferrer]

I had to change all the axis configuration because I realized before they were creating special axis only for the distribution plots surrounding subplot 1, i think you are gonna like this last figure:

[vinferrer]

disappointed so much for being a GFI.

With your permission I can open a PR for this non GFI @tsalo @eurunuela

[tsalo]

The figure looks great. Please open a PR when you have the time. Perhaps you could also include nipreps/fmriprep#19 in your changes? Namely, make plotting optional and making seaborn an optional dependency.

[vinferrer]

I think plotting is already optional. Of course i can add seaborn as a optional dependency, if i can find where to put that :laughing:

[tsalo]

It's optional, but done by default. We need to switch the default from making the plots to not making them.

[eurunuela]

It's optional, but done by default. We need to switch the default from making the plots to not making them.

Totally agree with this.

[XIXIYOUNG2018]

I almost have it, just need a bit of help on how to increase the first subplot size HI, I like the color and the figure you drawn, can you post the code for these fuigure? or at least told me the HEX or RGB value used. Thank you very much, it would be helpful.