Comments (32)
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 nipreps/fmripost-aroma#10 and nipreps/fmriprep#1784 have stalled (at least on my end).
from aroma.
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.
from aroma.
Here is an example of duplication that can be eliminated by using the hue
argument:
from aroma.
This is a great little task for anyone from Brainhack Donostia joining the project!
from aroma.
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?
from aroma.
Maybe we can implement here and if everything goes smoothly propose the changes in the original ICA-AROMA
from aroma.
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...
from aroma.
You meant something like this tsalo?
from aroma.
I can change the colours so they are the same as the others
from aroma.
Also there are a few deprecated messages
from aroma.
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.
from aroma.
from aroma.
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)
from aroma.
Yes, distplot
needs to be replaced with displot
or histplot
. I think either would probably be fine.
from aroma.
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).
from aroma.
Okay,
Yes,
distplot
needs to be replaced withdisplot
orhistplot
. I think either would probably be fine.
Actually we have a problem about this.
displot
is a figure-level function we cannot use it to plot the distribution plots in the axes.hisplot
doesn't have the vertical argument, which makes not possible to plot in vertical it seems.
Result:
from aroma.
@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.
from aroma.
@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.
from aroma.
😞 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.
from aroma.
And I don't want to pin our dependencies to an old version of seaborn, since that might impact the rest of our dependencies.
from aroma.
Well, we could generate the image separately and then plot in the subplot a png of that separated image
from aroma.
let my give a last try
from aroma.
I almost have it, just need a bit of help on how to increase the first subplot size
from aroma.
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
from aroma.
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.
from aroma.
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:
from aroma.
disappointed so much for being a GFI.
With your permission I can open a PR for this non GFI @tsalo @eurunuela
from aroma.
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.
from aroma.
I think plotting is already optional. Of course i can add seaborn as a optional dependency, if i can find where to put that 😆
from aroma.
It's optional, but done by default. We need to switch the default from making the plots to not making them.
from aroma.
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.
from aroma.
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.
from aroma.
Related Issues (20)
- Drop support for native-to-standard space transforms HOT 8
- [TST] We're missing automatic tests
- Update badges to new repo
- Add contributing guidelines and code of conduct
- Seaborn is not installed as dependency HOT 3
- CLI error: folder does not exist HOT 3
- Drop MELODIC step HOT 10
- Generate a benchmark dataset for integration tests HOT 6
- Too many parallel targets HOT 2
- Generate a benchmark dataframe for the `predict` function
- Generate a benchmark dataset for the features module HOT 6
- Transfer ownership to ME-ICA organization HOT 2
- Add FSL image for testing HOT 10
- Recognize contributions
- Move testing data into OSF HOT 1
- Change testing docker image HOT 2
- Integration test artifacts are not being stored HOT 1
- Test on Python 3.8 and 3.9
- Brainhack Donostia 2021 HOT 5
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from aroma.