Generalized Low Rank Models — Scientific Code Review

Paper: Madeleine Udell, Corinne Horn, Reza Zadeh, Stephen Boyd · published paper · arXiv 1410.0342
Code: madeleineudell/LowRankModels.jl · commit a18f0df45f1a

This review ran executable checks that compare the released code against what the paper states. Issues come first, grouped by importance: major, then minor. Where the released materials are too incomplete to check a claim, the finding says so in its text; such gaps are reproducibility limitations, not demonstrated errors. Findings whose fix is a direct revision of the paper's TeX or the released code include a Prompt to fix panel with a ready-to-use LLM prompt.

Major Findings

R1 [major] — Inconsistent Boolean Imputation and Error Encoding

The paper uses signed Boolean labels and imputes them with \operatorname{sign}(x_i y_j).

The released dataframe path maps levels to integer -1/+1 values in src/fit_dataframe.jl, but src/impute_and_err.jl returns Boolean false/true predictions and passes them to a same-type-only metric with the original numeric label. The prediction encoding therefore differs from the model encoding, and the Boolean-versus-integer metric call has no matching method.

This path cannot compute the released misclassification metric for the two signed-label protocols. Each reports 100 draws, so corrected results for the affected 200 draw-regime total require reruns.

Suggested fix: Use one signed -1/+1 representation throughout encoding, imputation, and error measurement, and add dispatch tests. Rerun both 100-draw protocols.

Work in the released LowRankModels.jl repository, project version 1.1.1, for Generalized Low Rank Models; the arXiv identifier was not supplied in the review evidence. The paper uses signed Boolean labels and sign(x_i y_j). In src/fit_dataframe.jl, the dataframe path maps levels to integer -1 and +1 values, but src/impute_and_err.jl returns Bool false and true and then calls a same-type-only metric with the original numeric label. Use one signed -1/+1 representation throughout encoding, imputation, and error measurement, and make the metric dispatch accept that representation. Add tests covering both prediction values and integer labels. Rerun the 100-draw synthetic Boolean PCA protocol and the 100-draw mixed-type protocol, then republish corrected aggregates.

R2 [major] — Unreachable Offset-Aware SVD Initialization

The paper's SVD initialization reserves an offset coordinate, fixes it to 1 in X, initializes it with column means in Y, reduces the effective rank, and applies the associated standard-deviation scaling.

In the released code, GLRM.rx is an array and add_offset! wraps each element with lastentry1, but src/initialize.jl tests typeof(glrm.rx) == lastentry1. That whole-array test is false, so the effective-rank decrement, the row of ones in X, the mean row in Y, and the tied scaling branch are skipped.

The released offset-enabled SVD path does not perform the initialization described in the paper. Corrected initialization trajectories and the MSQ plot require reruns.

Suggested fix: Detect the elementwise offset representation and add an offset-enabled init_svd! regression test. Rerun the initialization comparison and MSQ demonstration.

Work in the released LowRankModels.jl repository, project version 1.1.1, for Generalized Low Rank Models; the arXiv identifier was not supplied in the review evidence. GLRM.rx is an array in src/glrm.jl, add_offset! wraps each array element with lastentry1 in src/modify_glrm.jl, but the guard in src/initialize.jl compares typeof(glrm.rx) with the element type lastentry1. This makes the offset-aware init_svd! branch unreachable and skips the effective-rank decrement, X ones row, Y mean row, and tied standard-deviation scaling. Replace the whole-array type test with detection of the elementwise offset representation. Add an offset-enabled init_svd! regression test that checks all four consequences. Rerun the initialization comparison and the MSQ demonstration, and regenerate their reported trajectories and plot.

R3 [major] — Missing Reproduction Artifacts

The paper reports eight experiment families: synthetic Boolean PCA, censored Boolean recovery, mixed-type fitting and imputation, the 2013 American Community Survey experiment, model selection, initialization comparisons, the MSQ demonstration, and Spark scaling.

The released code/data contain no family with complete paper-matched inputs, configuration, seeds, environment, driver, and raw outputs. Three released examples are analogs with documented differences, and the separately preserved MSQ data from RDatasets.jl v0.8.1 are not identified as the version used for the paper.

The released materials do not allow any of the eight reported result families to be checked end to end. This is a reproduction gap, not a demonstrated error in the reported numbers.

Suggested fix: Publish the exact inputs and preprocessing, configuration grids, seeds, dependency and hardware environments, drivers, and raw outputs. Then rerun and republish all eight result families.

Minor Findings

R4 [minor] — Incorrect One-Sparse Euclidean Projection

The paper states that the proximal operator of an indicator function is Euclidean projection onto its feasible set.

Both OneSparseConstraint proximal methods in src/regularizers.jl use argmax(u). For u=[-3,2], the released rule keeps 2 and has squared distance 9, whereas Euclidean projection keeps -3 and has squared distance 4.

The released OneSparseConstraint operator is therefore not Euclidean projection. The review does not establish that a reported experiment used this constraint.

Suggested fix: Select argmax(abs.(u)) in both proximal methods and add negative-entry tests. Rerun any workflow that used OneSparseConstraint.

Work in the released LowRankModels.jl repository, project version 1.1.1, for Generalized Low Rank Models; the arXiv identifier was not supplied in the review evidence. In src/regularizers.jl, both OneSparseConstraint prox methods at lines 237-238 use argmax(u), which keeps 2 for u = [-3, 2] and gives squared distance 9, while Euclidean projection keeps -3 and gives squared distance 4. Change both methods to select argmax(abs.(u)) while preserving the selected signed value. Add negative-entry regression tests covering this counterexample. Rerun every workflow that used OneSparseConstraint and report any changed results.

R5 [minor] — Paper/Code Step-Size Inconsistency

The paper sets the initial row or column step to alpha/n_i, decreases it by 30\% when the objective increases, and increases it by 5\% when the objective decreases.

Both released optimizer paths divide by n_i+1. For n_i=1,2,10, their steps are respectively 1/2, 2/3, and 10/11 of the documented steps. The implemented decrease factor 0.7 and increase factor 1.05 agree with the paper.

The released optimizer therefore uses a different initial step rule. Its effect on reported results has not been recomputed and requires reruns.

Suggested fix: Make the implementation use the published n_i denominator, or revise the paper and release notes to document n_i+1. Add denominator tests and rerun experiments using this optimizer.

Work on Generalized Low Rank Models; the arXiv identifier was not supplied in the review evidence, and use the released LowRankModels.jl repository, project version 1.1.1. The paper specifies initial row and column step size alpha divided by n_i, but src/algorithms/proxgrad.jl and src/algorithms/proxgrad_multithread.jl divide by n_i + 1. For n_i = 1, 2, and 10, the released steps are 1/2, 2/3, and 10/11 of the documented steps. The 0.7 decrease and 1.05 increase factors already agree. Decide whether the intended rule is n_i or n_i + 1. If it is n_i, update both optimizer paths; if it is n_i + 1, revise the paper TeX and release notes to state that rule. Add tests for n_i = 1, 2, and 10, then rerun every reported experiment that used this optimizer and republish affected results.

R6 [minor] — Broken Shared-Memory Model Construction

The paper describes a shared-memory implementation that stores A, X, and Y in shared memory, partitions factor updates, synchronizes the workers, and lets a master process check convergence and adjust the step length.

In the released code, GLRM.rx has type Array{Regularizer,1}, while ShareGLRM.rx is a scalar Regularizer. The exported share(glrm::GLRM) adapter passes glrm.rx unchanged, so construction is type-incompatible before computation starts. The separate threaded fitting path is not this published SharedArray adapter.

Shared-memory computation cannot start through the released adapter, and no shared-memory experiment was executed through it.

Suggested fix: Make the ShareGLRM field and adapter agree on the row-regularizer collection. Add a construction-and-fit test and rerun the shared-memory examples.

Work in the released LowRankModels.jl repository, project version 1.1.1, for Generalized Low Rank Models; the arXiv identifier was not supplied in the review evidence. In src/glrm.jl, GLRM.rx is Array{Regularizer,1}. In src/shareglrm.jl, ShareGLRM.rx is scalar Regularizer, while share(glrm::GLRM) passes glrm.rx unchanged. This type mismatch blocks construction of the published shared-memory model. Make the ShareGLRM field and adapter use one consistent representation of the row-regularizer collection; if choosing a transformation instead of storing the collection, document and test its semantics. Add a construction-and-fit test through share(glrm::GLRM). Rerun the shared-memory examples and report any changed results.