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

Update Komoran 2.4 -> 3 #198

Merged
merged 15 commits into from
Jul 31, 2018
Merged

Update Komoran 2.4 -> 3 #198

merged 15 commits into from
Jul 31, 2018

Conversation

lovit
Copy link
Contributor

@lovit lovit commented Jul 18, 2018

  • Komoran 2.4 -> Komoran 3.0 으로 버전 업데이트 하였습니다.
  • Komoran 에 텍스트 파일 형태의 사용자 사전을 등록하는 기능을 추가하였습니다.

@coveralls
Copy link

coveralls commented Jul 18, 2018

Coverage Status

Coverage increased (+5.2%) to 56.567% when pulling c49e388 on komoran3 into 7667b92 on master.

Copy link
Member

@e9t e9t left a comment

Choose a reason for hiding this comment

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

PR 감사합니다!
v0.4.5로 릴리즈해보아요.

Ref #87
Resolves #107

@@ -78,20 +69,30 @@ def morphs(self, phrase):

return [s for s, t in self.pos(phrase)]

def __init__(self, jvmpath=None, dicpath=None):
def add_userdic_file(self, dicpath):
Copy link
Member

Choose a reason for hiding this comment

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

add_userdic_file을 별도의 method로 빼신 이유는 userdic을 여러 벌 추가하고 싶을 수 있기 때문인가요?
만일 그렇다면, __init__(self, jvmpath=None, dicpath=None, userdic=None)와 같이 initialization할 때 userdic을 입력받는 api는 어떨지 제안드려봅니다. 이 때 userdic에 wildcard를 포함한 input path를 받는다면 여러 파일도 한꺼번에 받을 수 있을듯 합니다.

한편, 사용 도중 userdic을 dynamic하게 추가하고 싶은 니즈도 있을지도 모르겠는데, 그런 니즈가 많을 것으로 예상되는 경우 제안하신 것처럼 별도의 method로 빼는 것도 좋을듯 합니다. (단, 이 경우 add_userdic으로 method 이름을 단순화하면 어떨까 싶네요!)

의견 부탁드립니다 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

사용자 사전을 추가하는 방식은 사전 파일을 추가하거나, 혹은 파일이 아닌 단어를 직접 추가할 수도 있기 때문이었기에 개별 단어를 추가한다면 dic_file 과 dic_words 를 구분하기 위해 userdic_file 이라는 이름을 이용했습니다.

그런데 pr 보낸 다음에 살펴보니 Komoran 에서 단어수준에서 사전을 추가하기 위해서는 Komoran source code 를 고쳐야 하네요.

그래서 konlpy.tag.Komoran 에는 사용자 사전 파일만 추가하는 기능만 구현하는게 좋겠다 싶고, 이때에는 제안해주신 init 함수 안에 dicpath 를 넣는 형태로 구현하며, _add_userdic 함수만 숨김으로 만들어두면 좋을듯 합니다.

더하여 pr 코드에서 기존에 init 에 들어있던 dicpath 를 modelpath 로 이름을 변경하였습니다. 코모란에서 학습된 사전을 modelpath 로, 사용자 사전을 dicpath 로 명명하면 두 데이터가 더 명확히 구분될 것이라 생각합니다.

Copy link
Member

Choose a reason for hiding this comment

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

아주 좋습니다!
그렇다면 api의 복잡도를 낮추기 위해 단어 추가는 고려하지 말고 파일 추가만 고려하는걸로 해요.

dicpath, modelpath를 구분한 것도 아주 좋습니다.

---------
dicpath : str
dictionary file path
"""
Copy link
Member

Choose a reason for hiding this comment

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

Docstring에 userdic이 어떤 형태를 가지는지, 예시가 추가되면 좋을 것 같습니다.

Copy link
Member

Choose a reason for hiding this comment

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

더불어 userdic을 사용하는 경우 어떻게 동작하게 되는지에 대한 예시도 있으면 좋을듯 합니다 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

commit c20426a 에 userdic 의 format 을 추가하였습니다.

@@ -13,28 +13,8 @@
__all__ = ['Komoran']


def parse(result, flatten, join=False):
Copy link
Member

Choose a reason for hiding this comment

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

코드가 훨씬 깔끔해졌네요! 👍

@e9t e9t merged commit 5b00015 into master Jul 31, 2018
This was referenced Aug 1, 2018
@e9t e9t deleted the komoran3 branch August 3, 2019 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants