Dag construction v2 #146
Dag construction v2 #146
Conversation
… the unsafe methods package-private
…e to pipeline, and added transformer methods so transformers can be chained w/o fit
… rewritten upon construction
…are rewritten right away
|
@etrain I've now responded to your comments & added unit tests |
|
@etrain let me know once this is good for me to take a look |
| /** | ||
| * Concats a Seq of DenseVectors into a single DenseVector. | ||
| */ | ||
| case class VectorCombiner[T : ClassTag]()(implicit zero: breeze.storage.Zero[T]) extends Transformer[Seq[DenseVector[T]], DenseVector[T]] { |
There was a problem hiding this comment.
Ack, 140 chars, will fix.
|
@shivaram how's this look now? |
| MaxClassifier | ||
|
|
||
|
|
||
| val predictor = Optimizer.execute(predictorPipeline) |
There was a problem hiding this comment.
Do we need to call the optimizer explicitly or is this just done here to get the dot file ?
There was a problem hiding this comment.
Whenever you want to use the optimizer it has to be called explicitly
There was a problem hiding this comment.
Does this mean we should add this to other pipelines as well now ? Or is it okay to not call the optimizer ? Also longer term we shouldn't expect the user to call this I guess, so we need to find a place to do this automatically
There was a problem hiding this comment.
Mmm... at the moment it will only make a difference in pipelines with > 1 estimator. We should add it to pipelines once we update them to construct the full DAGs before doing anything, but most of them currently don't.
As for when to call this automatically... still tbd, possibly when interpreting or compiling a pipeline, and @etrain may have some ideas.
|
Just some minor style comments. A couple of obligatory questions
|
|
Yup, the tests pass! |
|
Okay @shivaram, what about now? |
|
LGTM. Thanks @tomerk for working through this PR ! |
@etrain @shivaram Not yet ready for merging (and plenty of cleanup to be done), but feel free to start taking a look