search

cwatson / brainGraph

Graph theory analysis of brain MRI data
173 stars 51 forks source link

ENH: Alternative strategy avoiding complex structures #9

Closed richardbeare closed 4 years ago

richardbeare commented 5 years ago

Thanks for brainGraph.

I was exploring brainGraph to use in a slightly non standard structure and noticed that there was considerable code complexity in some of the helper structures checking for nesting structures etc, and nested loops to facilitate correct parameter settings. This PR, illustrates some tests of an alternative approach which eliminates the need for loop nesting and, I hope, offers reduced complexity in many situations. I couldn't see any steps that would be broken by this approach, but I haven't explored everything - see what you think.

The general idea is to stick everything in a table and operate on that instead of writing specialised code. There are plenty of R tools for creating the tables and, if you go that way, your code only needs to understand tables, not nesting structures. It is the idea underlying much of tidyverse.

There's also the option of going the direction of storing the graphs as a list column in a tibble, if you want, but I haven't tried that.

cwatson commented 5 years ago

Hi @richardbeare , thank you for your comment and contribution. I have taken a look at your code and actually had been working (a bit) on implementing something similar to what you wrote; i.e., using a table/helper function/etc. to simplify working with data. However, I am not sure I would like to incorporate the (exact) proposed approach, as it will require a lot of changes and testing to make sure everything interoperates correctly. As far as I remember, much of the "ugly" code is in the "user scripts" for which I give examples in the User Guide. Although I know that analysis_random_graphs is similar, but that was never written as a "true" function. In general I have tried to stick to the Unix philosophy, but as you have noticed I wasn't successful in many places.

I will think more about your proposal, and other ideas for brainGraph functions in the near future; I am pretty swamped at the moment with more pressing tasks.

richardbeare commented 5 years ago

Thanks for the interest. This PR is intended as a place holder and something to think about. There are lots of bits and pieces in brainGraph, and I haven't used them all yet, so I can't be sure that this idea would fit everywhere. Part of my thought was that, even as an extremely experienced R user (20+ years), and previous implementer of similar tools for python/networkx, I found it tricky to work my way through the user guide - thus a simplification/standardization of some of the approaches may ease the learning curve and increase uptake.

I am sure it will appear as though you are throwing away some existing code implementing this, and there will be largish changes required. However I suspect it will lead to a more flexible and easier to maintain code base. The one point I wasn't able to confirm myself was whether any of the post normalization and pre stats analysis steps needed to know about the grouping structures? Are you able to point out the areas where you think problems are most likely?

Happy to discuss further, and may have questions as I move my collaborators to use brainGraph instead of my old networkx code.

cwatson commented 5 years ago

Yes, I figured it was meant to be a placeholder.

I'm not surprised you found working through the User Guide to be tricky. It began as a dump of old code/workflows/ideas and has grown and changed as I add features and learn more, myself, about network analysis and R programming.

I have always been less-than-happy with all of the nesting, so I will definitely consider a more intuitive approach. I can't remember off the top of my head which functions need to know about the grouping structure, but I think mtpc is one of them. There are probably some others.

cwatson commented 4 years ago

Sorry for such a long delay, @richardbeare . I appreciate your pull request as it pushed me in a somewhat different direction. In the latest version (3.0.0), there is new functionality that reduces the issue of nesting especially, and I hope overall usability is much improved. I will close the pull request but feel free to share your thoughts.