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

Torch related naming #343

Open
brettviren opened this issue Sep 27, 2024 · 2 comments
Open

Torch related naming #343

brettviren opened this issue Sep 27, 2024 · 2 comments

Comments

@brettviren
Copy link
Member

brettviren commented Sep 27, 2024

Perhaps a minor thing but it's a source of confusion.

WCT has always had a bit of confusion with naming related to torch. There is the pytorch/ subpackage but it uses libtorch and not PyTorch. pytorch.org makes a distinction between these two source packages. Spack has py-torch recipe to install PyTorch. It includes some headers and libraries for libtorch but they do not seem to be complete.

Likeiwse, "libtorch" is used to name wcb --with-libtorch-* options and the LIBTORCH package label.

I propose this renaming:

  • torch/ as the sub-package directory
  • --with-torch* as the wcb CLI args
  • TORCH as the wcb env label
  • waft/torch.py as the waf tool
  • WireCellTorch/*.h as the includes
  • WireCell::Torch as the C++ namespace

I'm interested to learn of any show stopper problems with this proposal in total or in part.

@brettviren
Copy link
Member Author

I expect this sub-package to gain some prominence in the WCT dependency tree driven by SPNG work. This includes providing:

  • new IData and INode interface classes which themselves depend on torch::Tensor.
  • low-level functions depending on torch::Tensor
  • mid-level functions depending on util/ and iface/ as well as torch::Tensor.

It would be nice to resolve this naming issue prior to that new work, even if the resolution is to not change the name.

@brettviren
Copy link
Member Author

Some offline discussion concludes that indeed this is a good thing to cleanup.

I do not want to complicate the next release, which I believe is imminent, and will wait until after it is cut to do the renaming.

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

No branches or pull requests

1 participant