Maintain infrequent characters' order

[lukhnos]

User description

This pre-populates the phrases dictionary from the entries in BPMFBase.txt such that the dictionary will maintain the original insertion order (guaranteed since Python 3.7). The dictionary entries will then be overwritten with the actual data from PhraseFreq.txt.

This fixes #582.

How Tested

A diff of the resulting data.txt shows that only infrequently used characters are affected: https://gist.github.com/lukhnos/a93d72da8c77671b2b978f8ac4a3da42

---

PR Type

Bug fix, Enhancement

---

Description

• Pre-populates phrases dictionary with BPMFBase.txt entries to maintain order.

• Ensures infrequent characters' order matches traditional phonetic order.

• Fixes key mismatch errors during dictionary population.

• Updates cook.py to handle phrases dictionary more robustly.

---

Changes walkthrough 📝

	Relevant files
Bug fix	cook.py Pre-populate and maintain order in `phrases` dictionary    Source/Data/bin/cook.py Added pre-population of phrases dictionary with BPMFBase.txt entries. Ensured dictionary maintains insertion order for infrequent characters. Fixed key mismatch handling during dictionary population. Updated logic for processing PhraseFreq.txt data. +14/-1

---

Need help?

Type /help how to ... in the comments thread for any questions about PR-Agent usage.

Check out the documentation for more information.

Summary by Bito

This pull request fixes a bug in the McBopomofo input method by ensuring the correct ordering of infrequent characters. It modifies the `cook.py` script to pre-populate the `phrases` dictionary from `BPMFBase.txt` and resolves key mismatch errors, enhancing the script's robustness. Additionally, it preserves the traditional phonetic order for rarely used characters, utilizing Python 3.7+'s dictionary insertion order guarantee.

Unit tests added: False

Estimated effort to review (1-5, lower is better): 2

[bito-code-review]

Code Review Agent Run #adee72

Actionable Suggestions - 1

• Source/Data/bin/cook.py - 1

  • Consider consolidating duplicate validation blocks · Line 146-150

Review Details

• Files reviewed - 1 · Commit Range: 5439311..5439311

  • Source/Data/bin/cook.py
• Files skipped - 0
• Tools

  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful
  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful

---

AI Code Review powered by

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 582 - Partially compliant Compliant requirements: Ensure that infrequent characters' order in auto-selection matches the traditional phonetic order. Pre-populate the phrases dictionary with entries from BPMFBase.txt to maintain the original insertion order. Fix any key mismatch errors during dictionary population. Non-compliant requirements: [] Requires further human verification: []

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Possible Logic Issue The added logic for pre-populating the phrases dictionary with BPMFBase.txt entries (lines 127-132) and subsequent overwriting (lines 144-150) should be reviewed to ensure it does not introduce unintended side effects or performance issues. # Populate phrases dict with entries from BPMFBase.txt first: this is so # that the resulting phrases dict will maintain the order from # BPMFBase.txt; this is important for rarely used characters. for key in bpmf_chars: phrases[key] = UNK_LOG_FREQ while True: line = handle.readline() if not line: break if line[0] == '#': continue elements = line.rstrip().split() mykey = elements.pop(0) myvalue = elements.pop(0) try: readings = bpmf_phrases[mykey] except: sys.exit('[ERROR] %s key mismatches.' % mykey) phrases[mykey] = myvalue for (mykey, myvalue) in phrases.items(): try: readings = bpmf_phrases[mykey] except: sys.exit('[ERROR] %s key mismatches.' % mykey) Error Handling The error handling for key mismatches (lines 147-150) appears duplicated from earlier logic. Verify if this redundancy is necessary or if it can be optimized. try: readings = bpmf_phrases[mykey] except: sys.exit('[ERROR] %s key mismatches.' % mykey)

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
General	Avoid overwriting existing dictionary values Ensure that phrases[key] = UNK_LOG_FREQ is only executed if key does not already exist in the phrases dictionary to prevent overwriting existing values. Source/Data/bin/cook.py [130-131] for key in bpmf_chars: - phrases[key] = UNK_LOG_FREQ + if key not in phrases: + phrases[key] = UNK_LOG_FREQ Suggestion importance[1-10]: 8 Why: The suggestion ensures that existing values in the phrases dictionary are not overwritten when populating it with entries from bpmf_chars. This is a valid and impactful improvement as it prevents potential data loss and maintains the integrity of the dictionary.	8

[bito-code-review]

Changelist by Bito

This pull request implements the following key changes.

Key Change	Files Impacted
Bug Fix - Dictionary Order Preservation Fix	- cook.py - Modified dictionary population logic to maintain order of infrequent characters from BPMFBase.txt

[bito-code-review]

Code Review Agent Run #b72fb0

Actionable Suggestions - 0

Review Details

• Files reviewed - 1 · Commit Range: 7b710fc..d776acb

  • Source/Data/bin/cook.py
• Files skipped - 0
• Tools

  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful
  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful

---

AI Code Review powered by

[mjhsieh]

LGTM thanks!

[tianjianjiang]

@lukhnos sorry for being late to the party. +CC @mjhsieh

I think this PR nicely demonstrated the limitation of non-AST LLM-based code review.
and faster. I will make a new PR to emphasize improvements that should be there in my opinions.

For example, this initialization has a famous Pythonic pattern that is slightly safer

Suggested change

	for key in bpmf_chars:
	phrases[key] = UNK_LOG_FREQ
	phrases = {key: UNK_LOG_FREQ for key in bpmf_chars}