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

Replace old sparse -> dense -> sparse with new from_sparse method #235

Merged
merged 2 commits into from
Oct 9, 2023

Conversation

jkhouja
Copy link
Contributor

@jkhouja jkhouja commented Oct 5, 2023

Replaces all sparse torch casting to new method from_sparse except in 1 example where there's an issue #236 that's been documented.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@codecov
Copy link

codecov bot commented Oct 5, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (1178970) 96.54% compared to head (5118ee2) 96.54%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #235   +/-   ##
=======================================
  Coverage   96.54%   96.54%           
=======================================
  Files          58       58           
  Lines        2230     2230           
=======================================
  Hits         2153     2153           
  Misses         77       77           
Files Coverage Δ
topomodelx/utils/sparse.py 100.00% <100.00%> (ø)

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

@ninamiolane
Copy link
Collaborator

Tests need to pass before we can review. See failures here:
https://github.com/pyt-team/TopoModelX/actions/runs/6424391426/job/17444941942?pr=235

@jkhouja
Copy link
Contributor Author

jkhouja commented Oct 6, 2023

@ninamiolane Thank you for taking a look. I was about to tag you and @mhajij for your thoughts. There's a weird issue in can_train tutorial that I've been working on since yesterday.

TLDR. can_layer errors when accessing index of the sparse matrix. So need to call coalesce before that. But then it's causing a matrix dimension mismatch in MultiHeadLiftLayer during training which made me suspicious of something deeper going on:

File [~/workspace/code/TopoModelX/topomodelx/nn/cell/can_layer.py:273](https://file+.vscode-resource.vscode-cdn.net/Users/jkhouja/workspace/code/TopoModelX/tutorials/cell/~/workspace/code/TopoModelX/topomodelx/nn/cell/can_layer.py:273), in MultiHeadLiftLayer.forward(self, x_0, neighborhood_0_to_0, x_1)
    271     print(combined_x_1.shape)
    272     print(x_1.shape)
--> 273     combined_x_1 = torch.cat(
    274         (combined_x_1, x_1), dim=1
    275     )  # (num_edges, heads + in_channels_1)
    277 return combined_x_1

RuntimeError: Sizes of tensors must match except in dimension 1. Expected size 55 but got size 38 for tensor number 1 in the list.

So I'm tracing the source of the issue, identifying the matrix. It's taking me longer since I'm not familiar with CAN architecture. Will keep you posted.

@jkhouja
Copy link
Contributor Author

jkhouja commented Oct 6, 2023

@ninamiolane @mhajij I created a new ticket where I documented the details issue #236

@jkhouja jkhouja requested review from ninamiolane and mhajij October 8, 2023 11:16
@jkhouja jkhouja force-pushed the jude/use_new_sparse_method branch from ce1275e to 5118ee2 Compare October 8, 2023 11:19
@jkhouja jkhouja merged commit 62ee00c into main Oct 9, 2023
12 checks passed
@ffl096 ffl096 deleted the jude/use_new_sparse_method branch October 9, 2023 07:53
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