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

jkhouja commented 11 months ago

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[bot] commented 11 months ago

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

codecov[bot] commented 11 months ago

Codecov Report

All modified lines are covered by tests :white_check_mark:

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

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #235 +/- ## ======================================= Coverage 96.54% 96.54% ======================================= Files 58 58 Lines 2230 2230 ======================================= Hits 2153 2153 Misses 77 77 ``` | [Files](https://app.codecov.io/gh/pyt-team/TopoModelX/pull/235?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyt-team) | Coverage Δ | | |---|---|---| | [topomodelx/utils/sparse.py](https://app.codecov.io/gh/pyt-team/TopoModelX/pull/235?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyt-team#diff-dG9wb21vZGVseC91dGlscy9zcGFyc2UucHk=) | `100.00% <100.00%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ninamiolane commented 11 months ago

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 commented 11 months ago

@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 commented 11 months ago

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

pyt-team / TopoModelX

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

Codecov Report