SciNim / flambeau

Nim bindings to libtorch
85 stars 3 forks source link

try remove noinit #36

Closed Clonkk closed 1 year ago

Clonkk commented 2 years ago

Just noticed that the workaround to avoid templated declaration:

/home/clonkk/.cache/nim/test_accessors_slicer_d/@m..@s..@sflambeau@stensors.nim.cpp:34:9: error: ‘at::Tensor’ is not a template
   34 | typedef torch::Tensor<NF>  TY__PrdnljbGhfFRNRu5jD7C3g;
      |         ^~~~~

doesn't work on devel but work on v1.6.2 :/

Clonkk commented 1 year ago

https://github.com/nim-lang/Nim/pull/21378 related. removing zeroMem on init would with Flambeau

https://github.com/nim-lang/Nim/issues/21308

Clonkk commented 1 year ago

@Vindaar @HugoGranstrom

Is it reasonable to merge this once 2.0 is out and restrict Flambeau to 2.0 and above ?

Otherwise maintenance will be a lot of work and when defined since previous bugs were preventing us to have this properly working

HugoGranstrom commented 1 year ago

I'm all for Flambeau (and all other new/not yet officially published libraries in scinim) to require Nim v2.0 if they require it for our own sanity. In my mind Flambeau hasn't been officially released and is still beta, so making restrictions like this is totally fine by me.

Datamancer, Arraymancer, numericalnim, ggplotnim etc should still work with the old versions as long as it doesn't cause too much work to maintain alongside v2.0.

Vindaar commented 1 year ago

@Vindaar @HugoGranstrom

Is it reasonable to merge this once 2.0 is out and restrict Flambeau to 2.0 and above ?

Otherwise maintenance will be a lot of work and when defined since previous bugs were preventing us to have this properly working

Imo it would be perfectly fine to make sure the current master HEAD has a tag and then merge this PR with a version bump (0.x.y to 0.x+1.0) with the new Nim version requirements (which then currently would be devel's 1.9 or whatever that currently is with 2.0 RC released. Still is 1.9, no?)

Clonkk commented 1 year ago

Since we are all in agreement, I added the Nim version guard and I will merge as soon as PR is done :)