-
Notifications
You must be signed in to change notification settings - Fork 81
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
adding hypergraph/cell complex networks models/unit tests #182
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #182 +/- ##
==========================================
+ Coverage 97.71% 97.87% +0.15%
==========================================
Files 43 49 +6
Lines 1796 1790 -6
==========================================
- Hits 1755 1752 -3
+ Misses 41 38 -3
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! THere are some typos and misnamings, see my comments
topomodelx/nn/cell/can_bis.py
Outdated
from topomodelx.nn.cell.can_layer import CANLayer, MultiHeadLiftLayer, PoolLayer | ||
|
||
|
||
class CAN(torch.nn.Module): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mhajij this one should be called CANBis: the name of the python class always matches the name of the file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was is supposed to be deleted because it is not faithful to the original implementation and we have a better one in the repo. I am deleting it in the next push.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good
test/nn/cell/test_ccxn.py
Outdated
class TestCCXN: | ||
"""Test CWN.""" | ||
|
||
def test_fowared(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
test/nn/cell/test_cwn.py
Outdated
class TestCWN: | ||
"""Test CWN.""" | ||
|
||
def test_fowared(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also add the References section, in the docstrings, that cites the corresponding paper. You can copy paste the References section from the corresponding *_layer.py file.
Thanks @mhajij !
done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There might be some commits missing: some typos have not been resolved.
Could you fix the Github conflicts before pushing to the PR? Thanks
test/nn/cell/test_can.py
Outdated
class TestCAN: | ||
"""Test CAN.""" | ||
|
||
def test_fowared(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed across all files.
topomodelx/nn/cell/can_bis.py
Outdated
from topomodelx.nn.cell.can_layer import CANLayer, MultiHeadLiftLayer, PoolLayer | ||
|
||
|
||
class CAN(torch.nn.Module): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good
challenge CC networks unit tests.
challenge HG networks unit tests