Refactor proposal 1

[Clonkk]

@mratsim @HugoGranstrom

Following our discussion regarding a generic Tensor version :

The main idea would be to implement

type
  ATTensor {.importcpp: "Tensor", header: torchHeader.} = object # -> ATTensor can be an appropriate name for the Tensors bindings of Torch
  # RawTensor {.importcpp: "Tensor", header: torchHeader.} = object -> RawTensor is also a good name
  Tensor[T] = distinct ATTensor

And then add a generic proc for Tensor API (and make sure we replace the overloaded proc using ScalarKind by a generic version of it). Note that in later version, we can add other property of Tensor such as shape to the genericity.

For that, I suggest refactoring Flambeau folders a bit so we can still access the current non-generic version API :

Here is an idea :

• flambeau/ -> Top level folder for "idiomatic" exported stuff (generic tensors.nim, flambeau_nn.nim for instance).
• flambeau/sugar -> I just found that high_level wasn't descriptive enough).
• flambeau/helpers -> To factorize proc, macro, template to respect DRY principle
• flambeau/private -> high_level/internal but moved and renamed to private for consistency.
• flambeau/raw -> folders for everything directly related to the C++ API
  • flambeau/raw/bindings -> Raw bindings to C++ Torch API (current tensors.nim renamed to rawtensors.nim or attensors.nim)
  • flambeau/raw/cpp -> C++ STL related stuff

The idea is to contains the importcpp to the raw folder.

I believe this would give us the flexibility we need going forward while keeping the Torch related work to one repo since it is possible to do :

• In package level flambeau.nim

  import flambeau/tensors.nim
  import flambeau/sugar/[indexing, interop]
  export tensors, indexing, interop

• If you wish to work on RawTensors :

  import flambeau except tensors
  import flambeau/raw/bindings/attensors

[Clonkk]

We can also remove the raw folder and have flambeau/bindings, flambeau/cpp directly as the raw folder is in fact "empty".

The downside to it is that we don't isolate all `importcppcalls to one folder; also if future evolution have us maintain a generic API and a non-generic one having a dedicatedrawfolders gives us more flexibility to haveraw/helpersandraw/private`` for example (but maybe I'm overthinking this).

Alternatviely, the syntactic sugar part could be put in raw since it's based on the attensor / rawtensor type

[HugoGranstrom]

Depending on how we want to market the raw binding, as its own thing or as just that, raw binding. Depending on that I think we should choose the name of the raw Tensor. If we want it to be used as a separate unit then maybe a fancier name than RawTensor is suitable but otherwise, it is a pretty descriptive name of what it is. Not sure what I think about ATTensor though, TorchTensor sounds better in my ears then 🤔. I don't think shape should be part of the genericity though, it makes for a worse user-experience if you have to keep track of every single dimension all the time. Think @mratsim used that method in Arraymancer in the beginning but ditched it.

Regarding the structure, I like the approach where we totally separate the bindings/RawTensor into a folder of its own that has to be explicitly imported. And that the default import is the high-level library. And to keep things as future proof as possible if we decide to separate the parts in the future, I think that the flambeau/raw should be fully functional on its own so everything necessary should be imported by import flambeau/raw. And then I think it makes sense to have separate flambeau/sugar and flambeau/raw/sugar as they work with different datatypes.

[Clonkk]

I updated this PR with my "current" working version. It needs to be reviewed carefully, and possibly we need to write docstring for proc

[HugoGranstrom]

I haven't found that much that looks totally off in your code. It's hard for me to reason about the code right now for me with the noInit requirement, it feels like a bug could be staring me right in my eyes without noticing it 😛 So I think we can merge it but we'll have to write a bunch of tests to make sure it works as we expect it in many scenarios. And that we are very clear about this limitation. Recommending {.push noInit.} perhaps?

[Clonkk]

So I think we can merge it

Noted, I'll sit on it this week-end and review it as well after this week-end. Thanks for taking time to check it out.

we'll have to write a bunch of tests to make sure it works as we expect it in many scenarios. And that we are very clear about this limitation. Recommending {.push noInit.} perhaps?

Yeah, this limitation is not trivial. I wrote a comment on the Tensor type that explains a bit more and there is a test that demonstrate the issues with noinit zero-ing (i expanded a bit on it) the memory that triggers the ref counting in tests/raw/test_noinitensor.nim.

[HugoGranstrom]

Great! 👍

That comment helped me understand it better even :D