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

Fix for #25: CellDiff2Vec doesn't terminate #36

Merged
merged 3 commits into from
Oct 30, 2023

Conversation

jarpri
Copy link
Contributor

@jarpri jarpri commented Oct 30, 2023

The issue appears to be from complexes that are too small. The _run_diffusion_process method runs a while loop, comparing the number of nodes to diffusion_cover (which defaults to 80). For graphs with fewer than 80 nodes, this loop never terminates:

def _run_diffusion_process(self, node):
    """
    Generating a diffusion tree from a given source node and linearizing it
    with a directed Eulerian tour.

    Args:
        node (int): The source node of the diffusion.

    Returns:
        list of str: The list of nodes in the walk.
    """
    infected = [node]
    sub_graph = nx.DiGraph()
    sub_graph.add_node(node)
    infected_counter = 1
    while infected_counter < self.diffusion_cover:
        end_point = random.sample(infected, 1)[0]
        nebs = [node for node in self.graph.neighbors(end_point)]
        sample = random.choice(nebs)
        if sample not in infected:
            infected_counter += 1
            infected.append(sample)
            sub_graph.add_edges_from([(end_point, sample), (sample, end_point)])
            if infected_counter == self.diffusion_cover:
                break

To solve this I added an exception to CellDiff2Vec inside the fit method:

if self.diffusion_cover > g.number_of_nodes():
        raise ValueError(
            "The diffusion_cover is too large for the size of the graph.")

I didn't find any more issues with graphs meeting this requirement.

edit: Thank you Florian for spotting the issue with PathComplex! I reverted this change with rebase now.

@ffl096
Copy link
Member

ffl096 commented Oct 30, 2023

Note I have deleted all the references to PathComplex as I couldn't find this in the current version of TopoNetX. Please suggest otherwise if my steps weren't correct and I'm happy to edit this!

PathComplex is here. Looking at your fork of TopoNetX, you are quite behind on commits to the upstream repository. You should sync your fork to have the latest version: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

Please revert your changes regarding PathComplex afterwards. Thanks!

@codecov
Copy link

codecov bot commented Oct 30, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (85638fd) 94.63% compared to head (05b2320) 95.18%.
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #36      +/-   ##
==========================================
+ Coverage   94.63%   95.18%   +0.54%     
==========================================
  Files           8        8              
  Lines         149      166      +17     
==========================================
+ Hits          141      158      +17     
  Misses          8        8              
Files Coverage Δ
topoembedx/classes/cell_diff2vec.py 75.00% <ø> (+16.17%) ⬆️

... and 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mhajij
Copy link
Member

mhajij commented Oct 30, 2023

@jarpri can you please fix the lint before we merge ? Thanks!

@mhajij mhajij merged commit c0f0e60 into pyt-team:main Oct 30, 2023
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