Skip to content

Conversation

@smeeklai
Copy link
Contributor

@smeeklai smeeklai commented Mar 1, 2018

สวัสดีครับน้องต้นตาล

ผมปรับจุดที่แก้ไขได้ให้นิดหน่อยและช่วย improve ตรงส่วนของการตัดคำโดยใช้ custom dict ให้นิดหน่อยน่ะครับ

สิ่งที่แก้ไข:

  1. แก้ไขเลข version 1.5 -> 1.6
  2. ลบ f.close() ออกไป เพราะหากใช้ open แล้ว f จะปิดเองอัตโนมัต หลังออกจาก with statement
  3. พอดี code เก่ามันช้าตรงที่ ทุกครั้งที่เรียก dict_word_tokenize() แล้วมันเอา read file หรือ/และ เอา list of words ไปสร้าง trie ทุกครั้ง ซึ่งจริงๆแล้วมันไม่ได้จำเป็นและทำให้ช้ามากกว่าเรียกใช้ default dict หลายสิบเท่าเลย
    ผมก็เลยแก้ไขไฟล์ init.py ใน folder tokenize โดยเพิ่ม class Tokenizer ขึ้นมา ซึ่ง user จะ initiate object นี้แค่ครั้งเดียว พร้อมส่ง custom dict หรือใหม่ก็ได้ จากนั้นก็เรียกใช้ word_tokenize() ไปตัดคำได้เลย ตอนนี้ผมทำแค่ newmm ก่อน เพราะผมไม่กล้าแก้ code เก่ามากเกินไป เพราะมันค่อนข้างเปลี่ยน behevior ของตัว tokenize ไปเลย จากเมื่อก่อนแค่ import function แล้วก็สามารถเรียกใช้ได้เลย อยากจะฟังความเห็นน้องต้นตาลก่อนว่ายังไงดีแล้วค่อยช่วยแก้ไขให้ต่อไปครับ

ยังไงรบกวนน้องต้นตาล review อีกที ก็ได้ครับ ขอบคุณครับ ไม่เคย pull request มาก่อน ถ้าทำไรผิดยังไงก็ขอโทษด้วยนะครับ ยังไม่ได้ test กับ python2 ด้วยนะครับ

@coveralls
Copy link

coveralls commented Mar 1, 2018

Coverage Status

Coverage decreased (-1.7%) to 60.913% when pulling 028cb85 on smeeklai:improved_custom_dict_tokenization into ee4b1a1 on PyThaiNLP:dev.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 62.31% when pulling 6c8fa67 on smeeklai:improved_custom_dict_tokenization into 762fd0f on PyThaiNLP:dev.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 62.31% when pulling 6c8fa67 on smeeklai:improved_custom_dict_tokenization into 762fd0f on PyThaiNLP:dev.

@wannaphong
Copy link
Member

ความคิดเห็นผมนะครับ ตรงข้อ 3 เป็นไปได้ไหมครับ ที่จะไม่ต้องสร้าง class Tokenizer ใหม่ขึ้นมาครับ

@smeeklai
Copy link
Contributor Author

smeeklai commented Mar 3, 2018

@wannaphongcom ผมได้ลองหา workaround มาแล้ว และก็ได้วิธีที่สามารถให้ไม่ต้องสร้าง class ใหม่แล้วก็ได้ อาจจะให้เป็น temporary solution ไปก่อนก็ได้นะครับ แต่ในระยะยาว ผมสนับสนุนให้สร้าง class ดีกว่าครับ เพราะหากดูตาม design แล้ว ตัว engine แทบทุกตัวต้องใช้ Trie เป็น source ในการตัดคำ ฉะนั้นการสร้าง class ที่มี property trie อยู่น่าจะเป็นทางออกในระยะยาวที่ดีกว่านะครับ แล้วยังสามารถลดให้เหลือ function word_tokenize แค่อันเดียวได้ด้วยครับ

ถ้าน้องต้นตาล ok กับ workaround นี้แล้ว เดี๋ยวผมจะช่วยแก้ engines ที่เหลือ ให้ compatible กับทางออกใหม่นี้ให้ครับ

ขอบคุณครับ

@wannaphong
Copy link
Member

ขอบคุณครับ ผมลองแล้ว ok กับ workaround นี้ 👍

@wannaphong wannaphong self-requested a review March 3, 2018 12:36
@smeeklai
Copy link
Contributor Author

smeeklai commented Mar 7, 2018

@wannaphongcom น้องต้นตาลครับพอดีจากแนวทางที่แล้วที่ได้นำเสนอไป มันแอบมีประเด็นเรื่องของที่ว่า ในกรณีที่สมมุติไปเรียกใช้ pythainlp ใน jupyter notebook แล้วถ้าผมทำการแก้ไขคำศัพท์หรือเพิ่มเข้าไปในไฟล์ __filename__.txt แบบนี้แล้ว มันจะไม่ทำการ reload มาให้เพราะ custom_dict มันโดน initialized ค่าตั้งแต่ import แล้ว ทำให้ต้อง reset notebook ใหม่อย่างเดียวเลย

สุดท้ายผมก็เลยคิดว่าแนวทางล่าสุดนี้น่าจะดีที่สุด หากไม่ต้องการสร้าง class ก็คือผมได้สร้าง function create_custom_dict_trie ขึ้นมาเพื่อเอาไว้สร้าง trie ของตัวเองเพื่อที่จะส่งเข้าไปใน word_tokenize() เพราะใน word_tokenize() เพิ่ม args ตัวใหม่เข้ามาชื่อ custom_dict_trie เพื่อมารับตัว custom trie ที่ถูกสร้างขึ้นมา

ยังไงลองดู code ที่แก้ไขแล้วคิดเห็นยังไงบอกนะครับ

@wannaphong
Copy link
Member

wannaphong commented Mar 8, 2018

@smeeklai ถ้าดูที่

dict_word_tokenize(text,file='',engine="newmm",data=[''],data_type="file")

จะเห็นว่า มีส่วนพารามิเตอร์ data กับ data_type ครับ ผมว่าจะเขียนให้สามารถตัดคำที่ต้องการได้จากข้อมูล list ที่ใส่เข้าไปใน api ตัวนี้ด้วยครับ แต่ผมยังไม่มีเวลาทำต่อครับ น่าจะแก้ไขประเด็นข้างบนได้ด้วยการใช้ list แทนไฟล์ครับ

Copy link
Member

@wannaphong wannaphong left a comment

Choose a reason for hiding this comment

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

👍

@wannaphong wannaphong merged commit 26ec714 into PyThaiNLP:dev Mar 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants