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

Laser tensor PR rebase #477

Merged
merged 77 commits into from
Dec 10, 2020
Merged

Laser tensor PR rebase #477

merged 77 commits into from
Dec 10, 2020

Conversation

Vindaar
Copy link
Collaborator

@Vindaar Vindaar commented Nov 27, 2020

Here it is finally. Sorry for taking so long.

This is a rebase of the laser tensor PR #420 onto the current state of arraymancer. The major complication was the restructure of the whole arraymancer library to support importing individual parts as submodules.

I had to fix a few things that came up:

As discussed on #420 this can cause issues where certain types (variant objects) match different branches in the type definition and in the code using when T.supportsCopyMem.
For that reason I will define a type class of types for which we know that they support copyMem which will then be used in the code as well as in the type definition to make sure laser based tensors are used for the vast majority of types used in practice.

I'll finish the latter maybe today and at the latest over the weekend. Wanted to open this PR first though, because I don't know how much more time I have today and to see what the CI says...

@Vindaar
Copy link
Collaborator Author

Vindaar commented Nov 30, 2020

CI failure is because I forgot to add Complex to KnownSupportsCopyMem from here:
https://github.com/mratsim/Arraymancer/blob/laser-tensor-rebase/tests/tensor/test_filling_data.nim#L33

However, it's a bit worrying that this failed in the first place, indicating that non mem copyable types may have hidden bugs. It could be a good idea to run all basic tests in a mode where all types are treated as mem copyable? Maybe have a

when not defined(noPtrLenBackend):
type
  KnownSupportsCopyMem* = SomeNumber | char | Complex[float64] | Complex[float32]
else:
  KnownSupportsCopyMem* = distinct object

and then run tests with -d:noPtrLenBackend in addition?

edit:
I'm not sure if the test failure really is a failure or if the test is broken. copyFrom states that the two tensors should have the same shape:

  ## Copy the source tensor into the destination tensor.
  ## Both should have the same shape. If destination tensor is a view
  ## only the data exposed by the view is modified.

yet in the test they don't:

        let a = [[1,2],[3,4]].toTensor.reshape(2,2).astype(Complex[float64])
        var b = ones[Complex[float64]](4,1)

one is (2, 2), the other is (4, 1).

The thing is that the copyMem branch for contiguous tensors only performs a check on the size, but not the shape:

    if src.is_C_contiguous:
      assert dst.size == src.size
      omp_parallel_chunks(
            src.size, chunk_offset, chunk_size,
            OMP_MEMORY_BOUND_GRAIN_SIZE * 4):
        copyMem(
          dst.unsafe_raw_offset[chunk_offset].addr,
          src.unsafe_raw_offset[chunk_offset].unsafeAddr,
          chunk_size * sizeof(T)
        )

@mratsim: should copyMem perform an assert on the shape instead?
I'm going to fix the test case to use the same shape.

@Vindaar
Copy link
Collaborator Author

Vindaar commented Dec 2, 2020

FYI: nim-lang/Nim#16185 is still an open issue, which makes the code here unsafe if used with ARC.

And a personal blocker: ggplotnim does not pass its dataframe tests (independent of ARC!). It throws an out of memory error. I'll try to debug that in the coming days. Once I figure out if it's a problem on ggplotnim's or arraymancer's side, I'd consider merging this PR. @mratsim still has to review it at that point though.

mratsim and others added 25 commits December 9, 2020 21:07
…build/tests_tensor_part01 -r tests/_split_tests/tests_tensor_part01.nim"
later state:
- test_display
- test_higherorder
- test_ufunc
Commonalities:
- all rely on higher order templates map/apply
- all have at least a test that involve mapping a string
Issue can be reliably detected in its own module by wrapping tests into a proc and avoid globals and then calling GC_fullcollect or GC_collectZct
This is needed, due to the changes that happened to the Arraymancer
directory layout in the meantime.
This was already removed due to possibly being bugged in
56de8c9
`reset` sets the length of the underlying sequence to 0! This way we
actually reset it to empty and resize to tensor size, which should
reallocate a new block that will be zeroed for us.
Since `newSeq` already zeroes in the first place, we don't have to
call `setZero` when constructing a seq based tensor.
This at least _seems_ to work fine as far as I can test, both using
ARC and normal GC.

`export_tensor` still used `data` as well. For the time being we can
use `toRawSeq` until a better solution is in place.
As long as this test case passes, self assignment of objects
containing Tensors using ARC is broken in arraymancer!
- `io_hdf5` is not imported automatically anymore if the module is
  installed. The reason for this is that the HDF5 library runs code in
  global scope to initialize the HDF5 library. This means dead code
  elimination does not work and a binary will always depend on the
  HDF5 shared library if the `nimhdf5` is installed, even if not used.
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

Successfully merging this pull request may close these issues.

None yet

2 participants