Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KMeans 중심값 생성 함수 추가 (BallCut) #37

Merged
merged 17 commits into from Nov 28, 2020

Conversation

Seungeon-Kim
Copy link
Collaborator

구현내용

BallCut Algorithm

스크린샷 2020-11-28 오전 1 47 47

스크린샷 2020-11-28 오전 1 48 03

[참고자료]
https://lovit.github.io/nlp/machine%20learning/2018/03/19/kmeans_initializer/

논의사항

coverage distance 값 결정

화면 (선택)

스크린샷 2020-11-28 오전 1 36 40

Copy link
Collaborator

@rnfxl92 rnfxl92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

let distance = maxX - minX
let coverage = distance / 3

let result = kmm.initializeDistanceCentroids(k: 13, coverage: coverage, coordinates: centroids)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

함수명 달라진 것 같은데 괜찮나요??

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안괜찮아요! ㅋㅋㅋㅋㅋ 수정한 부분 커밋이 PR에 포함이안된것 같네요 죄송합니다

return centroids
}

var remove: [Coordinate] = []
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기 변수명 바꿔줘야 할 것 같아요!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indexForRemoveCentroids 어떨까요...?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removeCoordinates로 수정했습니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indexForRemoveCentroids 어떨까요...?

index를 담는 배열이 아니에요!!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ㅎㅎㅎ 죄송합니당 잘못봤네요

Sprint 2 automation moved this from To do to In progress Nov 28, 2020
Copy link
Collaborator

@eunjeongS2 eunjeongS2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생하셨습니다~~~

Comment on lines 22 to 25
var centroids: [Coordinate] = []
let container = createContainer(k: k, coordinates: coordinates)
centroids += pickCentroids(k: k, distance: coverage, container: container)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit : var centroids = pickCentroids(k: k, distance: coverage, container: container)
이렇게 하는건 어떤가용 ??

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋아요!


let topLeft = Coordinate(x: Mock.minX, y: Mock.maxY)
let bottomRight = Coordinate(x: Mock.maxX, y: Mock.minY)
let centroids = kmm.screenCentroids(topLeft: topLeft, bottomRight: bottomRight)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

메서드 명이 바뀐 거를 깜빡 하셨나 보네요.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵.. 바로 수정했습니다! 😅

@discardableResult
private func kmeansByRandom(k: Int, coords: [Coordinate]) -> [Cluster] {
let kmm = KMeans(k: 22)
let centroids = kmm.randomCentroids(rangeOfLat: Mock.minY...Mock.maxY,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기도요.

Copy link
Collaborator

@eunjeongS2 eunjeongS2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

클래스로 나누니까 보기가 훨씬 편한거같아용 ! 👍👍 고생하셨습니다 ~~

Comment on lines 22 to 26
func centroids() -> [Coordinate] {
let result: [Coordinate] = recursiveClassify(k: k, coordinates: points)

return result
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return recursiveClassify(k: k, coordinates: points) 로 줄일 수 있을거 같아용

Comment on lines 30 to 32
var centroids: [Coordinate] = []
let container = createContainer(k: k, coordinates: coordinates)
centroids += pickCentroids(k: k, distance: coverage, container: container)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기두 var centroids = pickCentroids(k: k, distance: coverage, container: container) 가 어떤지용

let container = createContainer(k: k, coordinates: coordinates)
centroids += pickCentroids(k: k, distance: coverage, container: container)

if centroids.count != k {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

guard는 어떠신가용

private func createContainer(k: Int, coordinates: [Coordinate]) -> [Coordinate] {
var coordinates = coordinates
var container: [Coordinate] = []
let mutiplier: Int = 5
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

매직넘버라면 전역으로 선언하는게 관리하기 편하지않을까용,,?

Comment on lines 75 to 83
var removeCoordinates: [Coordinate] = []

container.forEach { coordinate in
if centroid.distanceTo(coordinate) <= distance {
removeCoordinates.append(coordinate)
}
}

container = container.filter { !removeCoordinates.contains($0) }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이부분
container = container.filter { centroid.distanceTo($0) >= distance } 로 줄일 수 있을 거 같아용! 아닌가요,,?


import Foundation

final class BallCutCentroidGenerator: CentroidCreatable {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

제가 보기엔 모두 순수함수로 작성하셔서 struct도 괜찮을거같은데 어떠신가요??

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이부분은 의논을 해보죠. 매니저 클래스 만들고 결정해봐야할 것 같아요!

Copy link
Collaborator

@beggu84 beggu84 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

KMeansPerformanceTests.swift 가 프로젝트에서 빠져있는 거 같은데, 의도한 것 같네요ㅎㅎ
고생하셨습니다.

@Seungeon-Kim
Copy link
Collaborator Author

클래스로 나누니까 보기가 훨씬 편한거같아용 ! 👍👍 고생하셨습니다 ~~

세세하게 봐줘서 고마워요!

@Seungeon-Kim
Copy link
Collaborator Author

KMeansPerformanceTests.swift 가 프로젝트에서 빠져있는 거 같은데, 의도한 것 같네요ㅎㅎ
고생하셨습니다.

토요일에도 리뷰를 달아주셔서 감사합니다! 퍼포먼스 비교하는 부분은 아직 작성중이라서 일부러 빼놨는데 프로젝트파일에 포함되어있었나보네요 😂

@Seungeon-Kim Seungeon-Kim merged commit b62349f into develop Nov 28, 2020
Sprint 2 automation moved this from In progress to Done Nov 28, 2020
@Seungeon-Kim Seungeon-Kim deleted the feature/kmeans/initialize-centroids branch December 3, 2020 01:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

Centroids 초기값 함수 작성: k-means++
6 participants