pyt-team / TopoModelX

Topological Deep Learning
MIT License
219 stars 79 forks source link

210 docstrings #223

Closed jarpri closed 11 months ago

review-notebook-app[bot] commented 11 months ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

ninamiolane commented 11 months ago

@jarpri there is a github conflict with the can_train notebook. Github conflicts are not fun to solve on github. I'd recommend removing the can_train.ipynb from this PR and doing another PR for it. Your call though!

papamarkou commented 11 months ago

Thank you @jarpri. @ninamiolane, is this ready to be merged?

ffl096 commented 11 months ago

@jarpri You accidentally committed build and docs directories. Please revert that and rebase your branch to a history without these changes. We should avoid bringing this into the history of the main branch due to the images.

In case you don't know how to rebase a commit, we can take care of this as well by squashing all commits when merging. (To the person that merges this: The option "Squash and merge" is available under the arrow next to the merge button in Github's interface. I can take care of this as well.)

codecov[bot] commented 11 months ago

Codecov Report

All modified lines are covered by tests :white_check_mark:

Comparison is base (e6be4f1) 96.36% compared to head (7baf399) 96.36%. Report is 2 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #223 +/- ## ======================================= Coverage 96.36% 96.36% ======================================= Files 55 55 Lines 2036 2036 ======================================= Hits 1962 1962 Misses 74 74 ``` | [Files](https://app.codecov.io/gh/pyt-team/TopoModelX/pull/223?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyt-team) | Coverage Δ | | |---|---|---| | [topomodelx/base/aggregation.py](https://app.codecov.io/gh/pyt-team/TopoModelX/pull/223?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyt-team#diff-dG9wb21vZGVseC9iYXNlL2FnZ3JlZ2F0aW9uLnB5) | `100.00% <ø> (ø)` | | | [topomodelx/base/conv.py](https://app.codecov.io/gh/pyt-team/TopoModelX/pull/223?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyt-team#diff-dG9wb21vZGVseC9iYXNlL2NvbnYucHk=) | `100.00% <ø> (ø)` | | | [topomodelx/base/message\_passing.py](https://app.codecov.io/gh/pyt-team/TopoModelX/pull/223?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyt-team#diff-dG9wb21vZGVseC9iYXNlL21lc3NhZ2VfcGFzc2luZy5weQ==) | `96.22% <ø> (ø)` | | | [topomodelx/nn/cell/can.py](https://app.codecov.io/gh/pyt-team/TopoModelX/pull/223?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyt-team#diff-dG9wb21vZGVseC9ubi9jZWxsL2Nhbi5weQ==) | `89.28% <ø> (ø)` | | | [topomodelx/nn/cell/can\_layer.py](https://app.codecov.io/gh/pyt-team/TopoModelX/pull/223?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyt-team#diff-dG9wb21vZGVseC9ubi9jZWxsL2Nhbl9sYXllci5weQ==) | `100.00% <ø> (ø)` | | | [topomodelx/nn/cell/ccxn.py](https://app.codecov.io/gh/pyt-team/TopoModelX/pull/223?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyt-team#diff-dG9wb21vZGVseC9ubi9jZWxsL2NjeG4ucHk=) | `100.00% <ø> (ø)` | | | [topomodelx/nn/cell/ccxn\_layer.py](https://app.codecov.io/gh/pyt-team/TopoModelX/pull/223?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyt-team#diff-dG9wb21vZGVseC9ubi9jZWxsL2NjeG5fbGF5ZXIucHk=) | `100.00% <ø> (ø)` | | | [topomodelx/nn/cell/cwn.py](https://app.codecov.io/gh/pyt-team/TopoModelX/pull/223?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyt-team#diff-dG9wb21vZGVseC9ubi9jZWxsL2N3bi5weQ==) | `100.00% <ø> (ø)` | | | [topomodelx/nn/cell/cwn\_layer.py](https://app.codecov.io/gh/pyt-team/TopoModelX/pull/223?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyt-team#diff-dG9wb21vZGVseC9ubi9jZWxsL2N3bl9sYXllci5weQ==) | `100.00% <ø> (ø)` | | | [topomodelx/nn/hypergraph/allset.py](https://app.codecov.io/gh/pyt-team/TopoModelX/pull/223?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyt-team#diff-dG9wb21vZGVseC9ubi9oeXBlcmdyYXBoL2FsbHNldC5weQ==) | `100.00% <ø> (ø)` | | | ... and [40 more](https://app.codecov.io/gh/pyt-team/TopoModelX/pull/223?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyt-team) | |

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

papamarkou commented 11 months ago

@ffl096, if it would be possible to squash and merge, it would be the most efficient; rebasing the branch might be quite painful right now for @jarpri, due to not having repeated the process.

papamarkou commented 11 months ago

@ffl096, @ninamiolane, we communicated with @jarpri, who kindly offered to try in the next couple of days to do the rebasing (as a learning exercise). I recommended that Jaro makes a backup of his work, so that if anything goes wrong, he can simply resubmit a fresh PR. Let's give Jaro the chance to give it a go, and we may coordinate.

ffl096 commented 11 months ago

Don‘t worry, nothing is ever lost. If everything goes wrong, we can always find the orphan commit of the current state of this pr and restore it :)

jarpri commented 11 months ago

@ffl096, I've completed the rebase process. Could you please review the changes? I don't understand these conflicts. Thanks for your help and patience all!

papamarkou commented 11 months ago

@ffl096, @ninamiolane I can see @jarpri completed the changes you suggested and has done the rebase as well. Is this PR ok to merge, or are there any real pending conflicts?

papamarkou commented 11 months ago

Looks good to me! I am on holidays until next Wednesday, so if somebody else can resolve the conflicts and merge :)

Thank you @ffl096, I took a look at the conflicts. If I understand well, this is just the result of prining some docstrings only in the the notebooks. @ninamiolane, is this considered an issue, do we need to do anything about it, or is it ok?

papamarkou commented 11 months ago

@ffl096, just a gentle follow up if this could be merged?

ffl096 commented 11 months ago

This is ready to be merged now. No need to squash commits

papamarkou commented 11 months ago

This is ready to be merged now. No need to squash commits

Thank you very much @ffl096!

ffl096 commented 11 months ago

(Just for clarification: I can't merge anymore since I am now the last pusher to this pull request. Someone else has to hit the button)

papamarkou commented 11 months ago

Ok, I did it :) Thank you so much for your help @ffl096!