PyTorch wrapper for SchNet operations #40
Conversation
raimis
force-pushed
the
pytorch_schnet
branch
from
148ea0e to
9bc3607
Compare
raimis
force-pushed
the
pytorch_schnet
branch
2 times, most recently
from
46c9017 to
242a3cd
Compare
raimis
force-pushed
the
pytorch_schnet
branch
from
f72c076 to
b5f3f03
Compare
|
@peastman this is ready for the review. |
src/pytorch/CFConvNeighbors.h
Outdated
| using torch::Device; | ||
| using torch::Tensor; |
There was a problem hiding this comment.
The using keyword should never be used in a header file. If you do, it pollutes the namespace of any source file that includes it.
There was a problem hiding this comment.
These are within a namespace, so the mapping is torch::Device --> NNPOps::CFConvNeighbors::Device, not just Device. Also, this header is internal and would be needed for the public API.
There was a problem hiding this comment.
I still wouldn't do it. This is just one of those rules of C++ development that it's best to follow consistently. There's little to be gained from breaking it and a lot to lose. Once you write a header file, you never know where someone else might include that file in the future or what assumptions they'll have made.
|
This looks really good! I added just a few minor comments. |
|
Looks good to merge whenever it's ready. |
CFConvNeighborsCFConv