You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@spark.apache.org by johnnywalleye <gi...@git.apache.org> on 2014/07/07 16:16:39 UTC

[GitHub] spark pull request: fix bin offset in DecisionTree node aggregatio...

GitHub user johnnywalleye opened a pull request:

    https://github.com/apache/spark/pull/1316

    fix bin offset in DecisionTree node aggregations

    Hi, this pull fixes (what I believe to be) a bug in DecisionTree.scala.
    
    
    In the extractLeftRightNodeAggregates function, the first set of rightNodeAgg values for Regression are set in line 792 as follows:
    
    rightNodeAgg(featureIndex)(2 * (numBins - 2))
      = binData(shift + (2 * numBins - 1)))
    
    
    Then there is a loop that sets the rest of the values, as in line 809:
    
    rightNodeAgg(featureIndex)(2 * (numBins - 2 - splitIndex)) = 
      binData(shift + (2 *(numBins - 2 - splitIndex))) +
      rightNodeAgg(featureIndex)(2 * (numBins - 1 - splitIndex))
    
    
    But since splitIndex starts at 1, this ends up skipping a set of binData values.
    
    The changes here address this issue, for both the Regression and Classification cases.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/johnnywalleye/spark master

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/spark/pull/1316.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #1316
    
----
commit 73809dae4e02e1cf1e8a8c67fff1cf07376d8e15
Author: johnnywalleye <js...@gmail.com>
Date:   2014-07-07T14:04:22Z

    fix bin offset in DecisionTree node aggregations

----


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-2152][MLlib] fix bin offset in Decision...

Posted by manishamde <gi...@git.apache.org>.

Github user manishamde commented on the pull request:

    https://github.com/apache/spark/pull/1316#issuecomment-48414923
  
    @johnnywalleye Nevermind. I fixed the same error on the multiclass PR.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-2152] fix bin offset in DecisionTree no...

Posted by pwendell <gi...@git.apache.org>.

Github user pwendell commented on the pull request:

    https://github.com/apache/spark/pull/1316#issuecomment-48282497
  
    @johnnywalleye mind adding `[MLlib]` to the title. Then it's more likely to catch @mengxr's eye :)


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-2152][MLlib] fix bin offset in Decision...

Posted by manishamde <gi...@git.apache.org>.

Github user manishamde commented on the pull request:

    https://github.com/apache/spark/pull/1316#issuecomment-48506335
  
    Thanks @mengxr 


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-2152][MLlib] fix bin offset in Decision...

Posted by johnnywalleye <gi...@git.apache.org>.

Github user johnnywalleye commented on the pull request:

    https://github.com/apache/spark/pull/1316#issuecomment-48406001
  
    Great, thanks!
    
    
    On Tue, Jul 8, 2014 at 5:56 PM, manishamde <no...@github.com> wrote:
    
    > @johnnywalleye <https://github.com/johnnywalleye> I will review this PR
    > and get back soon.
    >
    > —
    > Reply to this email directly or view it on GitHub
    > <https://github.com/apache/spark/pull/1316#issuecomment-48404544>.
    >


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-2152][MLlib] fix bin offset in Decision...

Posted by manishamde <gi...@git.apache.org>.

Github user manishamde commented on the pull request:

    https://github.com/apache/spark/pull/1316#issuecomment-48409180
  
    LGTM! The classic off-by-one error gets you no matter how much attention you pay. Thanks for the rigorous review.
    
    @pwendell Could we also add this 1.0 patch release?
    
    @johnnywalleye If possible, could you also update this PR. https://github.com/apache/spark/pull/886 It's on my repo on branch 'multiclass'. It's easy for me to update this but I am hoping you could review it as well since it's a non-trivial change. :-)


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-2152][MLlib] fix bin offset in Decision...

Posted by manishamde <gi...@git.apache.org>.

Github user manishamde commented on the pull request:

    https://github.com/apache/spark/pull/1316#issuecomment-48404544
  
    @johnnywalleye I will review this PR and get back soon.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-2152] fix bin offset in DecisionTree no...

Posted by johnnywalleye <gi...@git.apache.org>.

Github user johnnywalleye commented on the pull request:

    https://github.com/apache/spark/pull/1316#issuecomment-48215065
  
    Thanks, I have edited the subject of this pull request and added comments in JIRA.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-2152][MLlib] fix bin offset in Decision...

Posted by mengxr <gi...@git.apache.org>.

Github user mengxr commented on the pull request:

    https://github.com/apache/spark/pull/1316#issuecomment-48422865
  
    Thanks @johnnywalleye for the fix and @manishamde for reviewing the PR! I merged it into both master and branch-1.0, but I'm not sure whether it can make into v1.0.1 . I will update the JIRA if it doesn't.
    
    Btw, I tested this patch locally but let's call Jenkins (sorry).


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---