Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b0c5b5ed3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (bestIndex < 0 || bestImprovement <= CoordinateUpdateTolerance) | ||
| { | ||
| break; |
There was a problem hiding this comment.
UpdateByGreedyCoordinateDescent compares bestImprovement against a fixed 1e-12, but that quantity is in squared-error units. If the same NMF problem is scaled down (for example, V, W, and H all multiplied by about 1e-7 in a balanced way), the relative residual is unchanged while every candidate step's improvement shrinks below this threshold, so Update becomes a no-op on otherwise valid low-magnitude inputs. This makes convergence depend on the units of the data rather than the geometry of the factorization.
Useful? React with 👍 / 👎.
| var denominator = fgram[index, index]; | ||
| if (denominator <= CoordinateDenominatorTolerance) | ||
| { | ||
| continue; |
There was a problem hiding this comment.
The denominator <= 1e-12 guard breaks the usual W -> cW, H -> H/c invariance of NMF. For a valid warm start with very small W columns and compensating large H rows, gram[i,i] falls below this cutoff and the corresponding H coordinates are never updated, while the same factorization at a different scale updates normally. In practice that can stall one half of the alternating step purely because the factors were rescaled, not because the optimization reached a stationary point.
Useful? React with 👍 / 👎.
1 participant
No description provided.