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: SCN and SCCNN breaking after TopoNetX code change on the up laplacian #207

Merged
merged 3 commits into from
Sep 19, 2023

Conversation

ninamiolane
Copy link
Collaborator

…p laplacian on simplicial complexes yet

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@ffl096
Copy link
Member

ffl096 commented Sep 19, 2023

Thanks for the fix, this was still on my to-do! I'll merge once tests have completed :)

@ninamiolane
Copy link
Collaborator Author

@ffl096 Awesome! This PR is only a quick fix, the better fix would be to allow 'weights' to be passed for the up laplacian: is this on your todo?

@ffl096
Copy link
Member

ffl096 commented Sep 19, 2023

Weights are not implemented at all in TopoNetX yet. I think that would be a larger tasks that I don't have time to work on right now. If nobody else picks this up in let's say two weeks, I can take a look at it. Lets keep track of this in pyt-team/TopoNetX#245.

For context: Before pyt-team/TopoNetX#242, the weight parameter was just silently ignored. The laplacian matrix never was weighted.

@ninamiolane
Copy link
Collaborator Author

Goooot it! Yes, I was wondering why the up laplacian was the only neighborhood structure for which it was failing. Good idea to keep track of this.

@codecov
Copy link

codecov bot commented Sep 19, 2023

Codecov Report

Patch coverage: 66.66% and project coverage change: -1.02% ⚠️

Comparison is base (1ece602) 97.29% compared to head (3805244) 96.27%.
Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #207      +/-   ##
==========================================
- Coverage   97.29%   96.27%   -1.02%     
==========================================
  Files          57       56       -1     
  Lines        2070     2042      -28     
==========================================
- Hits         2014     1966      -48     
- Misses         56       76      +20     
Files Changed Coverage Δ
topomodelx/base/message_passing.py 96.22% <ø> (ø)
topomodelx/nn/cell/can.py 89.28% <ø> (ø)
topomodelx/nn/cell/can_layer.py 100.00% <ø> (ø)
topomodelx/nn/cell/ccxn.py 100.00% <ø> (ø)
topomodelx/nn/cell/ccxn_layer.py 100.00% <ø> (ø)
topomodelx/nn/cell/cwn.py 100.00% <ø> (ø)
topomodelx/nn/cell/cwn_layer.py 100.00% <ø> (ø)
topomodelx/nn/hypergraph/allset.py 100.00% <ø> (ø)
topomodelx/nn/hypergraph/allset_layer.py 88.88% <ø> (ø)
topomodelx/nn/hypergraph/allset_transformer.py 100.00% <ø> (ø)
... and 31 more

... and 2 files with indirect coverage changes

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

@ninamiolane ninamiolane merged commit 0d2bdb6 into main Sep 19, 2023
6 of 8 checks passed
@ninamiolane ninamiolane deleted the ninamiolane-fix-error branch September 19, 2023 17:23
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.

None yet

2 participants