Conversation
bkleyn
left a comment
There was a problem hiding this comment.
Looks good to me at high-level. Added a few clarifying comments/questions.
Have you done any analysis to evaluate speed-up and/or improved scalability?
Signed-off-by: Xin Wang <xin.wang@fmr.com>
|
I am done with my first pass. To be direct, I am not comfortable with a PR that came a little too late in the year and just weeks before the AAAI Tutorial. This PR changes BOTH the API and the Behavior.. and with a tutorial ahead of us, for this PR to fly, it should have been crystal clear --which is unfortunately not the case. High-level comments:
|
Agree that this PR includes some major updates to the API and mining behaviors when it goes batching. I will send another note to the reviewers and facilitate discussions as the follow-ups to resolve comments. Given the AAAI tutorial will be in a few weeks, the PR would not be ready to be pushed through. I think it might be better to withdraw the PR and take the comments as follow-up actions. I will resubmit the PR once I have reviewers' questions and comments addressed more clearly. |
…into batch_processing
|
Hi All, I would like to reopen this PR for the consideration of adding batch processing capability. Since the last review, I made a few updates to the PR:
For the analysis on batch_size/discount_factor/runtime, we addressed the questions in last PR with more experiments. Here is a summary of analysis:
Let me know if you have more questions. |
skadio
left a comment
There was a problem hiding this comment.
LGTM. See some of comments before the final merge
|
Hi All, I believe all comments have been resolved. A few recent commits make these updates:
I plan to merge and have the new release either today or tomorrow. Thank you all again for the constructive comments! |
Signed-off-by: Xin Wang <xin.wang@fmr.com>
The PR is to enable pattern mining on batches of sequences instead of being only able to run on entire set. The changes included in this PR include: