Mainly entailed incorporating the new thrift API #78
Conversation
| (defn unpack-feature-vec [val neighbor-val] | ||
| (defn unpack-feature-vec [forma-val neighbor-val] | ||
| "TODO: Convert forma-val to thrift - see forma-seq | ||
| (schema/unpack-forma-val forma-val)" |
There was a problem hiding this comment.
Is this right? Don't we want (thrift/unpack forma-val) instead?
There was a problem hiding this comment.
you are right. but this is in the comment. it will be taken out.
There was a problem hiding this comment.
This is just in the doc. Dan removes it in another pull request he's working on.
There was a problem hiding this comment.
Sorry, may have misinterpreted what you wrote. If you mean that the whole function unpack-feature-vec should be in thrift, note that it's not unpacking a thrift object, it's unpacking the feature vector, which is filled with thrift objects. There may be a better way to do this but it's not related to the thrift api.
There was a problem hiding this comment.
Well, I'm actually wondering if unpack-feature-vec is needed. Like, what about just mapping unpack over the sequence?
(map thrift/unpack seq)Maybe?
There was a problem hiding this comment.
It doesn't look like it would work to just map thrift/unpack right now. But you're right that this function could be made a lot cleaner (and renamed, probably).
There was a problem hiding this comment.
Dan's right, there's a bit more than just using the thrift/unpacker going on here, and the feature vector has to be constructed exactly this way. It's definitely unwieldy though, so would be great to clean it up.
Create an issue?
There was a problem hiding this comment.
Yeah let's submit an issue for this.
|
LGTM. #shipit |
|
@eightysteele, if you think this looks good, maybe you could merge the pull request? Otherwise, let us know what is missing. |
|
@robinkraft, can you create an issue for |
|
#shipit On Fri, Jul 13, 2012 at 2:24 PM, Aaron Steele <
|
Incorporating thrift api into classify namespace
Updated forma.logistic.classify and forma.ops.classify to reflect new thrift API.
... Merging in specific namespaces from feature/deliver.