Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implementation of visualisation tools #121

Merged
merged 59 commits into from
Aug 14, 2019

Conversation

wingedRuslan
Copy link
Collaborator

@wingedRuslan wingedRuslan commented Jun 13, 2019

  • I'm ready to merge
  • What's the context for this pull request?
    (this is a good place to reference any issues that this PR addresses)

Visualisation tools
References issues #116 #115 and #119 #118

  • What's new?

I've implemented visualisation tools for the following purposes:

  1. plot_rich_club - for plotting the rich club values per degree along with the random rich club values created from a random network with a preserved degree distribution.

Result was before comments
Rich_club_values

Result now after comments
Rich_club_values

  1. plot_network_measures - plotting network measures values along with the random network values values created from a random network

Result
Barrr

  1. plot_degree_dist - plotting the degree distribution along with the degree distribution of an Erdos Renyi random graph

Result was before comments
3333Plot

Result now after comments (no gap)

degree_dist

Kirstie's code was really helpful but mostly I've designed these functions by myself, so there are might be some features(changes) you would like to have and on the contrary not to have.

Basically, I tried to follow the Kirstie's code (like to have the same parameters to the functions), but the main implementation was either hard to reproduce or had errors while running. That's why I relied on my intuition and created functions from my perspective.

Feel free to point out me any wrong decisions I took while implementing visualization tools.

I have also tried to have the same outputs illustrated in issue #1.

The way how to use created functionalities I showed in the visualisations_tutorial.

  • What should a reviewer feedback on?

The design of the functions and the produced figures

  • Does anything need to be updated after merge?
    (e.g the wiki or the WhitakerLab website)

Documentation is updated to include a new module - visualisations.py

@KirstieJane
Copy link
Member

This is SO COOL @wingedRuslan! Way to go!

I'll add some comments now 😸

Copy link
Member

@KirstieJane KirstieJane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work @wingedRuslan!

The main piece of work is to add the error bars so that you're plotting across multiple random networks rather than just one. The other comments are quite small.

I haven't had a full run through the jupyter notebook, but great work! I'll look at that at the next pass 😺


# save the figure if the location-to-save is provided
if figure_name:
fig.savefig(figure_name, bbox_inches=0, dpi=100)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the folder doesn't exist yet, does this work? I feel like it probably needs a os.makedirs check...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohhh, did not check that, I will modify code to handle this case

(e.g. color =["#06209c","#c1b8b1"]) or you can pass an (r, g, b) tuple,
where each of r, g, b are in the range [0,1]. Finally, legal html names
for colors, like "red", "black" and so on are supported.
labelBar : bool, optional
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style wise: label_bar rather than labelBar? To fit with the other parameters?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, I should be consistent in naming

network_measures : dict
real network measures
Note: the dict could be obtained from calculate_global_measures()
rand_network_measures : :dict
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this contain multiple random network measures? Or just one?

The plot needs error bars so that you can see the variance of the random networks.

Here's the original code that calculated the global measures for a bunch of random graphs: https://github.com/KirstieJane/NSPN_WhitakerVertes_PNAS2016/blob/5c9c46caf91768d4cadec2b24078b640f05d3d76/SCRIPTS/make_graphs.py#L274

This is the same issue as above (line 49)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one.
Yeah, I did not know that we need a bunch of random values. I've created functions to accept only 1 random values.

This is the same issue as above (line 49)

Right now plot_network_measures(network_measures, rand_network_measures..) accepts

network_measures : dict
        real network measures
rand_network_measures : dict (from 1 random graph)
        random network measure values

Should I redesign so that the function accepts rand_network_measures which is a list of some random measures?
Like:

{'assortativity': x1, x2, ... xn 
 'average_clustering': x1, x2, ... xn 
 'average_shortest_path_length': x1, x2, ... xn 
 'efficiency': x1, x2, ... xn 
 'modularity': x1, x2, ... xn }

And then plot with error bars, right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be ideal to accept the dataframe or dict returned by brain_bundle.report_global_measures() if you can figure that out

Islast
Islast previously requested changes Jul 31, 2019
Copy link
Collaborator

@Islast Islast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @wingedRuslan this is nice work. Really well done, I do however have two requests:

  • I have asked for some (very pedantic) changes over the real_network variable name.
  • There is a function that you seem to have deleted and incorporated into the place where it is used. I prefer more functions, but I'm happy to be challenged

from scona.helpers import save_fig


def plot_rich_club(brain_bundle, real_network, figure_name=None, color=None,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like #139 might be resolved

While instantiating GraphBundle object we pass the real Graph and its name.
(e.g. bundleGraphs = scn.GraphBundle([H], ['Real Network'])).
To plot rich club values along with the rich club values from randomed graphs
it is required to pass the name of the real network (e.g.'Real Network').
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm quite cautious about the notion of "real graph" that you use here. I would perhaps say

   real_network: str, required
        This should index the particular network in `brain_bundle` that you want the figure to highlight. A distribution of all the other networks in `brain_bundle` will be rendered for comparison.

I would also prefer to rename real_network to something like distinguished_network or main_network, because the word real raises certain assumptions

@wingedRuslan wingedRuslan requested a review from Islast August 3, 2019 15:33
`brain_bundle` will be rendered for comparison.
To plot rich club values along with the rich club values from
randomed graphs it is required to pass the name of the initial network,
the proper network, the one you got from the mri data.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to remove the following from all function descriptions:

       To plot rich club values along with the rich club values from
        randomed graphs it is required to pass the name of the initial network,
        the proper network, the one you got from the mri data.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

I find small_coefficient to be rather confusing...
I think small_world_coefficient makes much more sense.
I find small_coefficient to be rather confusing...
I think small_world_coefficient makes much more sense.
@KirstieJane KirstieJane changed the base branch from master to KW-gitignore August 8, 2019 07:26
@KirstieJane KirstieJane changed the base branch from KW-gitignore to master August 8, 2019 07:55
@KirstieJane
Copy link
Member

Hey @wingedRuslan! This all looks great to me! If you can merge wingedRuslan#6 then I'm totally fine for you to merge!

@Islast - you requested changes and I think they're all addressed. Would you mind taking a look and approving when you have a chance?

Copy link
Member

@KirstieJane KirstieJane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WOOOO! Well done @wingedRuslan 💖

Rename tutorial; visualisations_tutorial -> global_measures_viz
@wingedRuslan
Copy link
Collaborator Author

@KirstieJane, Thank you a lot! I would like to merge but as @Islast requested changes, her approval is needed.

@wingedRuslan wingedRuslan mentioned this pull request Aug 12, 2019
59 tasks
@KirstieJane KirstieJane dismissed Islast’s stale review August 14, 2019 09:05

Chatted with @Islast offline and she's happy to merge

@KirstieJane
Copy link
Member

Wooooo - click the big green button @wingedRuslan!! MERGE AWAY 🚀:star2:

@wingedRuslan wingedRuslan merged commit 76a639e into WhitakerLab:master Aug 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants