Skip to content

fix: uninitialized input matrix with thread <= 10 - #14

Open
bact wants to merge 6 commits into
munlicode:mainfrom
bact:fix/densematrix-uniform-init
Open

bact wants to merge 6 commits into
munlicode:mainfrom
bact:fix/densematrix-uniform-init

Conversation

@bact

@bact bact commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor
  • uniform() filled only thread/10 of the input matrix, plus no tail when thread=10; the rest stayed uninitialized -> NaN or non-deterministic training for thread<=10
  • Fill all 10 blocks + tail for any thread count (sequentially for thread=1). Values for thread>=11 are unchanged; thread<=10 now gets a fully initialized matrix, so its output differs from before
  • Applies to both random and pretrained-vector init

kUniformBlocks lets uniformThread and uniform share the block count.

The test runs _CHILD (a small script that imports fasttext) in 5 fresh processes and checks, for random and pretrained init across threads 1, 2, 10, 11, 12, that every value is initialized and the matrix is identical for every run.

- uniform() filled only thread/10 of the matrix; leave uninitialized -> NaN if thread<10
- Fill all blocks for any thread count; thread>=11 unchanged
- Covers pretrained path; add regression test

Signed-off-by: Arthit Suriyawongkul <[email protected]>
@bact

bact commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor Author

Before this PR (uses runs from PR 13 as examples):

With this PR:

munlicode

This comment was marked as outdated.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

A critical seed-overflow issue and several regression-test gaps remain unresolved.

Review effort: Lite
Findings: 1 High severity · 1 Medium severity

Open (2)
What changed in this PR

Fixes DenseMatrix::uniform so the input matrix is fully initialized for any thread count, with regression coverage.

Changes:

  • Distributes all matrix blocks across workers.
  • Shares block configuration between initialization paths.
  • Adds deterministic Python regression tests.
File Summary Review findings
src/​densematrix.cc Initializes every matrix block across thread counts. Critical: avoid signed overflow when deriving RNG seeds from INT32_MAX.
python/​fasttext_module/​fasttext/​tests/​test_matrix_init.py Adds initialization and determinism coverage. Moderate: allow valid zero values, use a small explicit bucket count, and cover threaded cases beyond thread=1.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/densematrix.cc Outdated
Comment thread python/fasttext_module/fasttext/tests/test_matrix_init.py Outdated
@bact

bact commented Oct 4, 2026

Copy link
Copy Markdown
Contributor Author

Thank you for the review. I will look at the comments and suggestions.

@bact

bact commented Oct 4, 2026

Copy link
Copy Markdown
Contributor Author

Fixed issues as suggested.

  • Ubuntu - 84 passed, 4 skipped
  • macOS - 84 passed, 4 skipped
  • Windows - 84 passed, 4 skipped

(the additional 4 passed are from the recent PRs merged)

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The claimed pretrained-vector initialization path is not covered by the regression test.

Review effort: Lite
Findings: 1 Medium severity · 1 Low severity

Open (2)
Resolved since last review (2)

Comment thread python/fasttext_module/fasttext/tests/test_matrix_init.py
Comment thread python/fasttext_module/fasttext/tests/test_matrix_init.py Outdated
bact and others added 2 commits October 6, 2026 14:24
Signed-off-by: Arthit Suriyawongkul <[email protected]>

Co-authored-by: Copilot Autofix powered by AI <[email protected]>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The test assertion does not fully validate the claimed determinism behavior.

Review effort: Lite
Findings: None

Resolved since last review (2)

@bact bact changed the title fix: fully initialize input matrix for any thread count fix: uninitialized input matrix with thread <= 10 Oct 6, 2026
@bact
bact requested a lite review from Copilot October 6, 2026 15:45

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approval recommended

Only a minor, non-blocking test nit remains.

Review effort: Lite
Findings: None

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