Github user haying commented on a diff in the pull request:
https://github.com/apache/incubator-madlib/pull/75#discussion_r87916186
--- Diff: src/modules/convex/algo/igd.hpp ---
@@ -86,6 +101,32 @@ IGD<State, ConstState, Task>::merge(state_type &state,
template <class State, class ConstState, class Task>
void
+IGD<State, ConstState, Task>::mergeInPlace(state_type &state,
--- End diff --
Hi, great to see you are using the convex framework to do this. I am trying
to understand why you need to add the transitionInPlace and mergeInPlace. As I
read the code, you can achieve what you are doing by using the old transition
and merge function but to use algo.incrModel anywhere you are using task.model
right now.
Of course, now you don't need to call final function to update the model.
But with that, now you cannot calculate the loss across the whole epoch using a
fixed model. It may or may not be an issue given the concrete use case.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
