Uploaded image for project: 'Flink'
  1. Flink
  2. FLINK-4148

incorrect calculation distance in QuadTree

    Details

      Description

      https://github.com/apache/flink/blob/master/flink-libraries/flink-ml/src/main/scala/org/apache/flink/ml/nn/QuadTree.scala#L105

      Because EuclideanDistanceMetric extends SquaredEuclideanDistanceMetric we always move in first case and never reach case for math.sqrt(minDist)

      correct match first EuclideanDistanceMetric and after it SquaredEuclideanDistanceMetric

      p.s. because EuclideanDistanceMetric more compute expensive and stay as default DistanceMetric it's can cause some performance degradation for KNN on default parameters

        Issue Links

          Activity

          Hide
          Zentol Chesnay Schepler added a comment -

          1.2: 9d99f2bd4a29b748905e55a774ff04f933b6b00f
          1.3: 6e920fcd6601b82e821f59fb0226eceeb9457ce6

          Show
          Zentol Chesnay Schepler added a comment - 1.2: 9d99f2bd4a29b748905e55a774ff04f933b6b00f 1.3: 6e920fcd6601b82e821f59fb0226eceeb9457ce6
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/flink/pull/2442

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/2442
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user Fokko commented on the issue:

          https://github.com/apache/flink/pull/2442

          Looks good, please merge. Should have been fixed long ago

          Show
          githubbot ASF GitHub Bot added a comment - Github user Fokko commented on the issue: https://github.com/apache/flink/pull/2442 Looks good, please merge. Should have been fixed long ago
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zentol commented on the issue:

          https://github.com/apache/flink/pull/2442

          I'm sorry, I completely forgot about this one. I will merge it with the next batch.

          Show
          githubbot ASF GitHub Bot added a comment - Github user zentol commented on the issue: https://github.com/apache/flink/pull/2442 I'm sorry, I completely forgot about this one. I will merge it with the next batch.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user nssalian commented on the issue:

          https://github.com/apache/flink/pull/2442

          Seems good to me. @zentol do you have time to add some extra review on this?

          Show
          githubbot ASF GitHub Bot added a comment - Github user nssalian commented on the issue: https://github.com/apache/flink/pull/2442 Seems good to me. @zentol do you have time to add some extra review on this?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user xhumanoid commented on the issue:

          https://github.com/apache/flink/pull/2442

          slave was killed without any notifications:

          ERROR: Maven JVM terminated unexpectedly with exit code 137
          Putting comment on the pull request
          Finished: FAILURE

          Show
          githubbot ASF GitHub Bot added a comment - Github user xhumanoid commented on the issue: https://github.com/apache/flink/pull/2442 slave was killed without any notifications: ERROR: Maven JVM terminated unexpectedly with exit code 137 Putting comment on the pull request Finished: FAILURE
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user nssalian commented on the issue:

          https://github.com/apache/flink/pull/2442

          Hi @xhumanoid , thanks for the PR. Could you please check the Failure messages and fix the build? Can help review once the PR is cleanly mergable.

          Show
          githubbot ASF GitHub Bot added a comment - Github user nssalian commented on the issue: https://github.com/apache/flink/pull/2442 Hi @xhumanoid , thanks for the PR. Could you please check the Failure messages and fix the build? Can help review once the PR is cleanly mergable.
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user xhumanoid opened a pull request:

          https://github.com/apache/flink/pull/2442

          FLINK-4148 incorrect calculation minDist distance in QuadTree

          Thanks for contributing to Apache Flink. Before you open your pull request, please take the following check list into consideration.
          If your changes take all of the items into account, feel free to open your pull request. For more information and/or questions please refer to the [How To Contribute guide](http://flink.apache.org/how-to-contribute.html).
          In addition to going through the list, please provide a meaningful description of your changes.

          • [ ] General
          • The pull request references the related JIRA issue ("[FLINK-XXX] Jira title text")
          • The pull request addresses only one issue
          • Each commit in the PR has a meaningful commit message (including the JIRA id)
          • [ ] Documentation
          • Documentation has been added for new functionality
          • Old documentation affected by the pull request has been updated
          • JavaDoc for public methods has been added
          • [ ] Tests & Build
          • Functionality added by the pull request is covered by tests
          • `mvn clean verify` has been executed successfully locally or a Travis build has passed

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

          $ git pull https://github.com/xhumanoid/flink master

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

          https://github.com/apache/flink/pull/2442.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 #2442


          commit 4291a9ac78ec2bb989ffce4bbfd43e58b7d7b3b9
          Author: Alexey Diomin <diominay@gmail.com>
          Date: 2016-07-04T15:13:11Z

          FLINK-4148 incorrect calculation minDist distance in QuadTree


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user xhumanoid opened a pull request: https://github.com/apache/flink/pull/2442 FLINK-4148 incorrect calculation minDist distance in QuadTree Thanks for contributing to Apache Flink. Before you open your pull request, please take the following check list into consideration. If your changes take all of the items into account, feel free to open your pull request. For more information and/or questions please refer to the [How To Contribute guide] ( http://flink.apache.org/how-to-contribute.html ). In addition to going through the list, please provide a meaningful description of your changes. [ ] General The pull request references the related JIRA issue (" [FLINK-XXX] Jira title text") The pull request addresses only one issue Each commit in the PR has a meaningful commit message (including the JIRA id) [ ] Documentation Documentation has been added for new functionality Old documentation affected by the pull request has been updated JavaDoc for public methods has been added [ ] Tests & Build Functionality added by the pull request is covered by tests `mvn clean verify` has been executed successfully locally or a Travis build has passed You can merge this pull request into a Git repository by running: $ git pull https://github.com/xhumanoid/flink master Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/2442.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 #2442 commit 4291a9ac78ec2bb989ffce4bbfd43e58b7d7b3b9 Author: Alexey Diomin <diominay@gmail.com> Date: 2016-07-04T15:13:11Z FLINK-4148 incorrect calculation minDist distance in QuadTree
          Hide
          humanoid Alexey Diomin added a comment -

          yes, I will make PR

          Show
          humanoid Alexey Diomin added a comment - yes, I will make PR
          Hide
          neelesh77 Neelesh Srinivas Salian added a comment -

          Alexey Diomin, thank you for reporting this. Could you please create a Pull Request for the same patch?

          Show
          neelesh77 Neelesh Srinivas Salian added a comment - Alexey Diomin , thank you for reporting this. Could you please create a Pull Request for the same patch?
          Hide
          humanoid Alexey Diomin added a comment -

          small patch

          Show
          humanoid Alexey Diomin added a comment - small patch

            People

            • Assignee:
              Unassigned
              Reporter:
              humanoid Alexey Diomin
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development