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

[WIP] [BREEZE-590] DenseTensor Implementation #695

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@sujithjay
Copy link

sujithjay commented Feb 25, 2018

#590
The idea of this patch is to implement a DenseTensor, as the title suggests. It is still a work in progress; however, I decided to raise a PR anyways to elicit comments. I am still very new to Breeze and could do with some guidance, and a thorough review from someone with experience in the codebase.

The primary design choice I made is to choose TensorN over Tensor3, Tensor4 etc. This does give away static type safety, but brings in flexibility in choosing dimensions of the Tensor.

I have in mind the following checklist to complete this patch in the current form:

  • Implement UFuncs
  • Improve the iterator performance (by adding a cache, probably?)
  • Exhaustive tests

Please feel free to suggest more items to add to the checklist. Thank you.

@dlwh
Copy link
Member

dlwh left a comment

so sorry for the slow response. Thanks for taking this on!

This has the general right shape (heh) for a dynamically-shaped tensor. The biggest problem is that it's too slow of an implementation, and it's important to me that Breeze generally be fast.

More specifically:

  • IndexedSeq[_] (being backed by scala.collection.Vector) are horrifically inefficient, and we should replace that with a better IndexedSeq that's just backed by an Array.
  • The use of HOFs is idiomatic Scala, but unfortunately Scala's implementation of them is so slow that imperative code should be preferred for hot spots (and apply is likely to be a hot spot)

The formatting's also not standard Scala, mostly because of the positions of elses.

I'm also just unsure of how to go about implementing the "tensor-y" functions. I'm specifically interested in the generalization of matrix multiplication as well as dimension-reduction operations (e.g. summing out two dimensions of the tensor)

.forall{ idx =>
idx._2 > -shape(idx._1) && idx._2 < shape(idx._1)
}
if(!isValidIndex) throw new IndexOutOfBoundsException("")

This comment has been minimized.

@dlwh

dlwh Mar 12, 2018

Member

isValidIndex should be extracted out as a method

}
}

logicalIndex * (stride + 1) + offset

This comment has been minimized.

@dlwh

dlwh Mar 12, 2018

Member

why is this stride + 1?

In any event, I don't think that an element-by-element stride isn't really that useful for higher-order tensors. In practice, I think you'd you want a per-dimension stride. (Or, alternatively, the stride only to apply to the "major" axis, as in DenseMatrix)

}
}

def +(elem: IndexedSeq[Int]): Set[IndexedSeq[Int]] = throw new UnsupportedOperationException(" member '+' is not supported in TensorN.keySet() ")

This comment has been minimized.

@dlwh

dlwh Mar 12, 2018

Member

this can just coerce to a set with iterator.toSet + elem

@sujithjay

This comment has been minimized.

Copy link
Author

sujithjay commented Mar 18, 2018

Thank you for reviewing the PR, @dlwh . I hope to make the relevant changes, in accordance to the review comments, sometime this week.

@sujithjay

This comment has been minimized.

Copy link
Author

sujithjay commented Mar 18, 2018

IndexedSeq[_] (being backed by scala.collection.Vector) are horrifically inefficient, and we should replace that with a better IndexedSeq that's just backed by an Array.

To do this, would you prefer me introducing a new type, say immutable.ArraySeq, as part of breeze.collection and use that in stead of immutable.IndexedSeq; or another approach would be to transform the IndexedSeq into an Array-backed one within the DenseTensorN implementation using something like this:

def ASeq[T](elt: T*)(implicit ct: ClassTag[T]): IndexedSeq[T] = {
   val a = elt.toArray.clone
   a.deep.asInstanceOf[IndexedSeq[T]]
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment