Add an opt-in override for the fewer-observations-than-assets check

[sstoeckl]

Reworked around the option you described, as a single commit. The original
version of this pull request removed the check; this one leaves it in place and
adds a way past it.

statistical.factor.model(R, k = 1, ..., allow.fewer.obs = FALSE)

With the default, nothing changes. The same check, the same message, the
same results. tests/testthat/test-stat-factor-model.R is untouched and passes
unchanged on this branch — that file is the evidence, and modifying it was the
thing the earlier version could not avoid.

With the override the fit proceeds and k is bounded by m - 2. That is
where the arithmetic binds: extractCovariance() divides the residual sums of
squares by m - k - 1, so k = m - 1 returns an infinite covariance rather
than an error. The bound lives inside the m < N branch, so the m >= N path
is not touched in any respect. The number of assets does not enter it, because
on this path there are more assets than observations.

Documentation

A Details section covering, in order: why the restriction is the default,
what a short wide panel does to sample second moments, when lifting it is
defensible, and what it costs. References on both sides.

For: Connor and Korajczyk (1986, 1988), whose asymptotic principal components
are built for a cross-section that grows against a fixed time series, which is
the opposite of the usual asymptotics and the reason the method exists; Bai and
Ng (2002) for choosing k in that regime; Fan, Liao and Mincheva (2013) for
the covariance estimator built on top of it.

Against: Michaud (1989) on error maximization, Ledoit and Wolf (2004) and Fan,
Fan and Lv (2008) on why regularization exists, DeMiguel, Garlappi and Uppal
(2009) on losing to equal weighting out of sample.

The section says plainly that the covariance returned is full rank by
construction, which is not the same as being well estimated, and that the
option is not there to obtain an invertible matrix from data that does not
support one.

Deliberately not in this change

Anything on the m >= N path, including k = m - 1 there, which still returns
Inf. And the truncation of a non-integer k by as.integer(), which behaves
the same on master: k = 0.5 becomes 0 and extractCovariance() returns
NULL. Both are separate decisions.

Tests

A new file, tests/testthat/test-factor-model-wide.R, with ten tests pinning
both sides: the default refuses with the message it always had, with the
argument absent and with it FALSE; the opt-in path fits, is bounded at
m - 2, and returns a finite covariance at every admissible k; too few
observations for any factor are refused; k <= 0 is refused first; a
positional third argument still reaches prcomp(); and the tall and square
cases are identical with the option set either way.

No existing test is modified.

	PASS	FAIL	ERROR
test-stat-factor-model.R, master	49	0	0
test-stat-factor-model.R, this branch	49	0	0
test-factor-model-wide.R, master	7	5	5
test-factor-model-wide.R, this branch	28	0	0

Test suite

The full testthat run on this branch is 3578 passing, with 2 failures and 2 errors, against 3550 passing on
master, with the same two failures and two errors on both sides:
chart.EfficientFrontierCompare in test-charts-roi.R and objective() in
test-objective-branches.R. Neither file is touched here.

Scope

Three files, 299 insertions, 3 deletions: the function, its generated .Rd,
and the new test file.

The argument name is a guess at your conventions. Happy to rename it, or to
change what the default's message says, if you would prefer something else.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

Existing regression expectations still fail, and invalid non-integer factor requests must be rejected before coercion.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Updates statistical.factor.model() to support universes with more assets than observations by validating feasible factor counts.

Changes:

• Replaces the m < N rejection with a data-supported k bound.
• Adds wide-universe, boundary, covariance, and regression tests.
• Verifies unchanged behavior for tall datasets.

File summaries

File	Summary
tests/testthat/test-factor-model-wide.R	Adds wide-universe and factor-boundary coverage.
R/stat.factor.model.R	Updates factor-count validation.

Review details

Suppressed comments (1)

R/stat.factor.model.R:70

• The new bound is applied after k has been truncated by as.integer(), so a non-integer request such as k = 0.5 becomes 0, passes k > max_k, and produces an stfm whose k is zero even though 1:k still selects a component later (making extractCovariance() return NULL). Validate that the original value is a finite positive integer before coercing; otherwise this guard does not enforce the contract stated by the existing error message.

  max_k <- min(m - 2L, N)
  if(max_k < 1L)
    stop("at least three observations are needed to fit a factor model")
  if(k > max_k)
    stop(sprintf(paste("k = %d is more factors than the data supports:",
                       "%d observations and %d assets give at most %d"),
                 k, m, N, max_k))

• Files reviewed: 2/2 changed files
• Comments generated: 2
• Review effort level: Lite

---

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

[Copilot]

🔵 Needs a closer look

Add coverage for the asset-count (N) upper bound, including k = N and k = N + 1.

Review details

Suppressed comments (1)

tests/testthat/test-factor-model-wide.R:37

• The new min(m - 2L, N) check's N limit is not exercised by this test: with wide (m = 5, N = 8), every assertion has max_k = 3, so even k = 99 only verifies the observation bound. Add a tall fixture with m - 2 >= N and assert that k = N succeeds while k = N + 1 receives the new validation error; otherwise regressions in the asset-count bound can pass.

test_that("k is bounded by the components the data supports, not by N", {
  m <- nrow(wide)
  # min(m - 2, N) = 3 here. extractCovariance() divides by m - k - 1, so
  # k = m - 1 would return an infinite covariance instead of an error.
  expect_no_error(statistical.factor.model(wide, k = m - 2L))

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Address the square-case compatibility change and clarify the user-facing error message.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

R/stat.factor.model.R:69

• This is a user-facing error message, but “k = %d is more factors” is grammatically incorrect and makes the failure harder to read. Please phrase it as a request/excess, for example “k = %d requests more factors than the data supports”.

R/stat.factor.model.R:64

• This bound also rejects a previously successful square case: with m = N and k = N, the old guard allowed the full PCA basis, which reconstructs x and makes extractCovariance() return the finite sample covariance (the residual numerator is zero even though the denominator is -1). That is different from the documented k = m - 1 zero-denominator/Inf case, so either preserve this compatibility case or explicitly document and test this additional breaking change.

  max_k <- min(m - 2L, N)

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite

[braverock]

Thanks for submitting a well formatted pull request.

I understand what you are trying to do, and what you want. That said, just because you CAN do something doesn't mean that you SHOULD. The check is there for a reason, and follows most standard statistical guidance. Fewer observations than variables introduces much higher risks of spurious correlations looking significant, and of stationarity risks already present in financial time series being even worse.

We could discuss adding an option (default FALSE) to override the current restriction, but there is no way we're going to remove it without guardrails. Adding that option would also need to come with roxygen documentation updates that discuss the option, why it is FALSE by default, and some curated guidance on when/why you would set it to TRUE, what the risks are, and citations for that guidance on both sides of the issue.

Happy to continue discussing.

[sstoeckl]

That is a fair objection, and the option you describe is the better design. I
have rebuilt the pull request around it, as a single commit, and rewritten the
description.

statistical.factor.model(R, k = 1, ..., allow.fewer.obs = FALSE)

With the default nothing changes: the same check, the same message, the same
results. The clearest evidence is that tests/testthat/test-stat-factor-model.R
is untouched here and passes unchanged — modifying it was the thing my earlier
version could not avoid, and that alone was a good reason to be suspicious of
it.

With the override the fit proceeds and k is bounded by m - 2, inside the m < N
branch only, because extractCovariance() divides the residual sums of squares
by m - k - 1 and k = m - 1 would return an infinite covariance rather than an
error. Nothing on the m >= N path is touched.

The Details section carries what you asked for: why the restriction is the
default, what a short wide panel does to sample second moments, when lifting it
is defensible, what it costs, and citations on both sides. For the option,
Connor and Korajczyk (1986, 1988), whose asymptotic principal components are
built for a cross-section that grows against a fixed time series; Bai and Ng
(2002) for choosing k in that regime; Fan, Liao and Mincheva (2013) for the
covariance estimator built on it. Against casual use, Michaud (1989), Ledoit
and Wolf (2004), Fan, Fan and Lv (2008), and DeMiguel, Garlappi and Uppal
(2009). It states plainly that the covariance is full rank by construction,
which is not the same as being well estimated, and that the option is not there
to obtain an invertible matrix from data that does not support one.

Deliberately left alone: everything on the m >= N path, including k = m - 1
there, which still returns Inf; and the truncation of a non-integer k by
as.integer(), which behaves the same on master. Both are separate decisions and
I did not want to smuggle them in here.

The argument name is a guess at your conventions. Happy to rename it, or to
change what the default's message says, if you would prefer something else.

[braverock]

Thanks for the updates here, and thanks for the rigor on the references. Once we get clean tests in the pipeline, I'll run it myself and merge.