-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
[Example] Add Pytorch Geometric Example #4568
Conversation
Hello @tchaton! Thanks for updating this PR. There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2020-11-18 16:50:21 UTC |
Codecov Report
@@ Coverage Diff @@
## master #4568 +/- ##
======================================
- Coverage 93% 93% -0%
======================================
Files 117 117
Lines 8949 8949
======================================
- Hits 8319 8316 -3
- Misses 630 633 +3 |
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.
I'm hoping that lightning logo will make its way into the trainer, would be neat :)
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.
minor formating
Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>
Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>
Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>
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.
LGTM, cool example
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.
can we pls add these: @tchaton
- needed packages to
requirements/examples.txt
- test run, even GPU only, how large is the dataset?
from torch_geometric.data import NeighborSampler | ||
from lightning import lightning_logo, nice_print | ||
except Exception: | ||
HAS_PYTORCH_GEOMETRIC = False |
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.
HAS_PYTORCH_GEOMETRIC = False | |
PYTORCH_GEOMETRIC_AVALAIBLE = False |
to be the same as everywhere
|
||
def prepare_data(self): | ||
path = osp.join( | ||
osp.dirname(osp.realpath(__file__)), "..", "..", "data", self.NAME |
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.
Let's move this path to the module top as constant, bc if you move this file/package one level up it does not work...
or see and use similar as in tests.__init__
var PACKAGE_ROOT
self.dataset = Planetoid(path, self.NAME, transform=self._transform) | ||
self.data = self.dataset[0] | ||
|
||
def create_neighbor_sampler(self, batch_size=2, stage=None): |
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.
shall we ad types or just create an issue for adding later?
it would be useful to add docs here as it seems to be very specific step
and create a NamedTuple Object. | ||
""" | ||
|
||
usual_keys = ["x", "edge_index", "edge_attr", "batch"] |
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.
can we rather get names from the TensorBatch
definition?
Batch: TensorBatch = namedtuple("Batch", usual_keys) | ||
return ( | ||
Batch( | ||
self.data.x[batch[1]], | ||
[e.edge_index for e in batch[2]], | ||
None, | ||
None, | ||
), | ||
self.data.y[batch[1]], | ||
) |
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.
I am sorry, just not sure about this notation, not sure what it really does...
|
||
# utils from Lightning to save __init__ arguments | ||
self.save_hyperparameters() | ||
hparams = self.hparams |
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.
any special reason? :]
return F.log_softmax(self.lin2(x), -1) | ||
|
||
def step(self, batch, batch_nb): | ||
typed_batch, targets = self.gather_data_and_convert_to_namedtuple(batch, batch_nb) |
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.
typed_batch, targets = self.gather_data_and_convert_to_namedtuple(batch, batch_nb) | |
typed_batch, targets = self._gather_data_and_convert_to_namedtuple(batch, batch_nb) |
is this good as public API?
update: I see later that it assigned from outside, then it is a but confusion, can we make a placeholder in init as
self.gather_data_and_convert_to_namedtuple = ...
parser.add_argument("--groups", type=int, default=16) | ||
parser.add_argument("--dropout", type=float, default=0.8) | ||
parser.add_argument("--cached", type=int, default=0) | ||
parser.add_argument("--jit", default=True) |
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.
parser.add_argument("--jit", default=True) | |
parser.add_argument("--jit", type=bool, default=True) |
@@ -0,0 +1,31 @@ | |||
def nice_print(msg, last=False): |
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.
Let's move this to PL_examples root?
name = "lightning-geometric" | ||
version = "0.1.0" | ||
description = "TorchScripted Pytorch Geometric Examples with Pytorch Lightning" | ||
authors = ["Thomas Chaton <thomas.ai@grid.com>"] |
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.
still the mail here...
authors = ["Thomas Chaton <thomas.ai@grid.com>"] | |
authors = ["Thomas Chaton <thomas@pytorchlightning.ai>"] |
* add example for Pytorch Geometric * remove hydra * add docstring * remove description * rename folder * update script to not break test * remove .lock * add Pytorch Geometric to doc * add docstring at the begining * add comments * Update pl_examples/pytorch_ecosystem/pytorch_geometric/README.md Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com> * Update pl_examples/pytorch_ecosystem/pytorch_geometric/README.md Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com> * Update pl_examples/pytorch_ecosystem/pytorch_geometric/cora_dna.py Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com> * add toml Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com> Co-authored-by: Jirka Borovec <jirka@pytorchlightning.ai>
some fixes come in #5085 |
What does this PR do?
This PR adds Pytorch Geometric Examples with Lightning.
Fixes # (issue)
Before submitting
PR review
Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In in short, see following bullet-list:
Did you have fun?
Make sure you had fun coding 🙃