search

BlueBrain / AstroVascPy

Vasculature blood flow computation and impact of astrocytic endfeet on vessels
https://astrovascpy.rtfd.io
Apache License 2.0
8 stars 2 forks source link

Fix connected component #12

Closed nico-canta closed 11 months ago

nico-canta commented 12 months ago

With this PR we solve the problem that appears when we work with a disconnected graph.
In order to explain the bug, let's consider the following graph (each line is an edge of the graph):

image

We can see that the nodes {0,1} are not connected with the main graph component i.e. nodes {2,3,4,5,6}.

On the main branch, when we load the graph we remove immediately the disconnected components.
https://github.com/BlueBrain/AstroVascPy/blob/8725b827706116fa60d817feacf51b55b835b439/astrovascpy/io.py#L68

However, this results in an error when we call some methods of the PointVasculature class from the vascpy library. (our graph is a PointVasculature object). For example:

graph_path = "./ML20200201_no_deg_2_point_vasc_cc.h5"  # for the path see the bug report on jira
graph = load_graph_from_h5(graph_path) 
graph.degrees

ValueError: row index exceeds matrix dimensions

This happens because internally an AdjacencyMatrix is created with size graph.edges.max()+1. (which is 6+1=7 in our example). But the graph has only five nodes {2,3,4,5,6}. Nodes 0 and 1 are missing.

The approach of vascpy seems correct, since it is a convention to start labeling nodes from 0, with no missing numbers. So, the fix does two things:

UPDATE: The GRAPH_HELPER class has been replaced by the utils.Graph class in #15

bbpbuildbot commented 12 months ago

Logfiles from GitLab pipeline #166500 (:no_entry:) have been uploaded here!

Status and direct links:

bbpbuildbot commented 12 months ago

Logfiles from GitLab pipeline #166503 (:no_entry:) have been uploaded here!

Status and direct links:

bbpbuildbot commented 11 months ago

Logfiles from GitLab pipeline #167516 (:white_check_mark:) have been uploaded here!

Status and direct links:

tristan0x commented 11 months ago

Does this PR need a review? Not clear to know if this is still a draft or not: the PR is not in draft mode but its title starts with Draft

bbpbuildbot commented 11 months ago

Logfiles from GitLab pipeline #167620 (:white_check_mark:) have been uploaded here!

Status and direct links:

nico-canta commented 11 months ago

Does this PR need a review? Not clear to know if this is still a draft or not: the PR is not in draft mode but its title starts with Draft

It is almost ready. I need to test a few more things. When it is ready I will request you to review it. Thank you.

bbpbuildbot commented 11 months ago

Logfiles from GitLab pipeline #167685 (:no_entry:) have been uploaded here!

Status and direct links:

bbpbuildbot commented 11 months ago

Logfiles from GitLab pipeline #167764 (:white_check_mark:) have been uploaded here!

Status and direct links:

nico-canta commented 11 months ago

Ok. I just finished testing with the mouse dataset and it works! This GRAPH_HELPER approach is the only thing that I came up with. It allows us to calculate the degrees and the main connected component only once, without having to change the current interface and the vascpy library.

nico-canta commented 11 months ago

@StephLisa I added you as a reviewer as well such that you can see what I changed.

bbpbuildbot commented 11 months ago

Logfiles from GitLab pipeline #167863 (:white_check_mark:) have been uploaded here!

Status and direct links:

tristan0x commented 11 months ago

Could you take some time to explain in the pull-request description what this contribution is doing @nico-canta ? The title mention a fix, can you also describe the problem this change resolves?

nico-canta commented 11 months ago

Could you take some time to explain in the pull-request description what this contribution is doing @nico-canta ? The title mention a fix, can you also describe the problem this change resolves?

Yeah. I've just updated the description.

StephLisa commented 11 months ago

@StephLisa I added you as a reviewer as well such that you can see what I changed.

@nico-canta thank you! I checked your changes and run the compute_static_flow_pressure.py on the mouse graph with cc and it works :) Can we do a quick zoom? I will try the new on the disconnected mouse graph as well.

bbpbuildbot commented 11 months ago

Logfiles from GitLab pipeline #168126 (:white_check_mark:) have been uploaded here!

Status and direct links:

bbpbuildbot commented 11 months ago

Logfiles from GitLab pipeline #168145 (:white_check_mark:) have been uploaded here!

Status and direct links:

nico-canta commented 11 months ago

Thank you @tristan0x for reviewing and for proposing the utils.Graph approach. @StephLisa with the latest changes, basically, our graph is an instance of the class utils.Graph. This new class is described in the first comment of the PR #15. Please let me know if everything is ok for you, and then I can merge this PR. Thank you.

StephLisa commented 11 months ago

@tristan0x and @nico-canta . Many thanks! @nico-canta, just a quick question. Is there a need since the last modifiations to modify the way we load the graph in the files we use to compute the flow (static and OU) in the examples folder?

nico-canta commented 11 months ago

@tristan0x and @nico-canta . Many thanks! @nico-canta, just a quick question. Is there a need since the last modifiations to modify the way we load the graph in the files we use to compute the flow (static and OU) in the examples folder?

No need to to modify anything. The functions for loading the graph are already updated.