Skip to content

portfolio: build the fair-draw weights on the sampler's own backend (backport from rift_O4d) - #168

Open
oshaughnessy-junior wants to merge 2 commits into
oshaughn:rift_O4cfrom
oshaughnessy-junior:rift_O4c_portfolio_fairdraw_backend
Open

portfolio: build the fair-draw weights on the sampler's own backend (backport from rift_O4d)#168
oshaughnessy-junior wants to merge 2 commits into
oshaughn:rift_O4cfrom
oshaughnessy-junior:rift_O4c_portfolio_fairdraw_backend

Conversation

@oshaughnessy-junior

Copy link
Copy Markdown

Backport of the rift_O4d fix in oshaughnessy-junior#76. Two commits, separable if you'd rather take them apart.

1. Plugin discovery guard (prerequisite)

import RIFT.integrators.mcsamplerPortfolio currently raises whatever an optional plugin raises, because pipeline.load() is unguarded. On every environment available here that is:

ModuleNotFoundError: No module named 'nflows'

from the optional NF pipeline — so --sampler-method portfolio is unselectable and mcsamplerPortfolio untestable on any install without torch/nflows. This is a hard prerequisite: without it the fair-draw code below cannot be reached or tested on this branch at all. rift_O4d already carries this guard; this is the same four lines.

2. The fair-draw backend fix

The fair-draw block builds the weights with the module-global converter but draws with self.xpy, and the two are independent:

wt = xpy.exp(identity_convert_togpu(ln_wt))
indx_list = self.xpy.random.choice(self.xpy.arange(len(wt)), size=n_extr, replace=True, p=wt)

identity_convert_togpu is cupy.asarray whenever cupy imports, while self.xpy is numpy for any sampler whose driver did not set it (MCSampler.__init__ defaults it to numpy) and under --sampler-xpy numpy, which sets sampler.xpy = numpy while set_xpy_to_numpy() only rebinds local variables and leaves the module globals on cupy. When they disagree, ln_wt goes to the device, numpy.exp dispatches through cupy's __array_ufunc__ and returns a cupy array, and numpy.random.choice is handed a device array as p=:

TypeError: Implicit conversion to a NumPy array is not allowed. Please use `.get()` to construct a NumPy array explicitly.

That is an abort, not a degraded result — the traceback runs analyze_event -> sampler.integrate -> integrate_log, so the ILE process dies and no extrinsic samples are written.

Two parts, matching the O4d fix:

  1. Weights on self.xpy, never the module-global backend.
  2. Gather on the host_rvs entries need not share a backend with the index array (sample_n is written through the instance identity_convert_togpu, which the ILE sets to cupy.asarray, while the aggregated keys arrive host-typed), and indexing a numpy array with a cupy array raises the same TypeError.

Plus one line beyond that: the three _rvs operands are host-converted before the arithmetic one line above, since numpy.array(<cupy>) raises identically one line earlier.

Severity differs from O4d — stated plainly

On rift_O4d, integrate_log additionally forces self.xpy = numpy (the portfolio aggregates on the host), so the two backends always disagree and --sampler-method portfolio cannot run on a GPU host at all. That is where this was measured: ldas-pcdev13, extrinsic-collapse demo at rho_net 146.8, 6/6 replicates of --sampler-portfolio AV,GMM died at this line, and 6/6 complete with the fix.

This branch leaves self.xpy alone, so the abort is conditional on the two backends disagreeing rather than unconditional. Same defect, narrower trigger. I did not reproduce it end-to-end on an O4c GPU run — the evidence here is the unit reproduction plus the O4d campaign.

Testing

New test/test_portfolio_fairdraw_backend.py: 5 passed, 3 skipped (numpy 1.24.4 env). All five runnable tests fail without commit 2.

A _DeviceArray stand-in reproduces the two cupy behaviours the bug turns on (ufunc dispatch via __array_ufunc__, __array__ raising), so the tests hit the exact production traceback — same mtrand.pyx frame, same message — on a CPU-only host rather than skipping.

Two tests skip on this branch, and the reason is recorded in the code. Driving the instance converter device-typed trips a separate, pre-existing defect in draw(), which converts member draws with self.identity_convert_togpu and then assigns them into host-typed buffers — so it raises inside draw(), long before the fair draw. That is untouched here (rift_O4d solves it by aggregating on the host) and it would be wrong to attribute it to the fair draw. The skip is conditional on the traceback actually passing through draw(), so these re-enable themselves if that is ever fixed. The gather is pinned regardless by the device-typed index test, which does run.

The test file deliberately differs from the O4d copy in its prose and in those two skips, because the branches genuinely differ in how self.xpy is bound.

Note

There is an unpushed local branch (rift_O4c_portfolio_usable) carrying a similar plugin guard alongside other O4c portfolio work. This PR stands alone against rift_O4c and does not depend on it; if that branch lands first, commit 1 here will likely conflict trivially or become redundant.

🤖 Generated with Claude Code

oshaughnessy-junior and others added 2 commits August 11, 2026 13:42
Plugin discovery calls pipeline.load() unguarded, so `import
RIFT.integrators.mcsamplerPortfolio` raises whatever the plugin raises.  On every
environment here that is

    ModuleNotFoundError: No module named 'nflows'

from the optional NF pipeline -- which makes --sampler-method portfolio
unselectable, and mcsamplerPortfolio untestable, on any install without torch/nflows.
Prerequisite for the fair-draw fix that follows: without it that code cannot be
reached or tested at all on this branch.

Guard each load and report the skip, as rift_O4d already does.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The fair-draw block BUILDS the weights with the module-global converter but DRAWS
with self.xpy, and the two are independent.  identity_convert_togpu is cupy.asarray
whenever cupy imports, while self.xpy is numpy for any sampler whose driver did not
set it (MCSampler.__init__ defaults it to numpy) and under --sampler-xpy numpy,
which sets sampler.xpy = numpy while set_xpy_to_numpy() leaves the module globals on
cupy.  Whenever they disagree, ln_wt went to the device, numpy.exp dispatched
through cupy's __array_ufunc__ and returned a cupy array, and numpy.random.choice
was handed a device array as p=:

    TypeError: Implicit conversion to a NumPy array is not allowed.
               Please use `.get()` to construct a NumPy array explicitly.

That is an abort, not a degraded result: the traceback runs analyze_event ->
sampler.integrate -> integrate_log, so the ILE process dies and no extrinsic samples
are written.

Backport of the rift_O4d fix.  There integrate_log ADDITIONALLY forces self.xpy =
numpy (the portfolio aggregates on the host), so the backends always disagree and
--sampler-method portfolio could not run on a GPU host at all: measured on
ldas-pcdev13 with the extrinsic-collapse demo at rho_net 146.8, 6/6 replicates of
--sampler-portfolio AV,GMM died at this line and 6/6 complete with the fix.  This
branch leaves self.xpy alone, so the trigger is narrower -- same defect, conditional
rather than unconditional.

Two parts, as in the O4d fix:

  * build the weights on self.xpy, never on the module-global backend; and
  * gather on the HOST -- _rvs entries need not share a backend with the index array
    (sample_n is written through the INSTANCE identity_convert_togpu, which the ILE
    sets to cupy.asarray, while the aggregated keys arrive host-typed), and indexing
    a numpy array with a cupy array raises the same TypeError.

Also host-converts the three _rvs operands before the arithmetic one line above, for
the same reason: numpy.array(<cupy>) raises identically one line earlier.

The regression suite uses a _DeviceArray stand-in reproducing the two cupy behaviours
the bug turns on, so it hits the exact production traceback on a CPU-only host.  All
five runnable tests fail without this change.

Two tests SKIP on this branch, with the reason recorded in the code: driving the
INSTANCE converter device-typed trips a separate, pre-existing defect in draw(),
which converts member draws with self.identity_convert_togpu and then assigns them
into host-typed buffers.  That is untouched here (rift_O4d solves it by aggregating
on the host) and would be wrong to attribute to the fair draw.  The gather itself is
pinned regardless by the device-typed INDEX test, which does run.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@oshaughnessy-junior

Copy link
Copy Markdown
Author

Heads-up on an overlap, and a question about how you'd like it handled.

I opened #172, the driver-side half of portfolio selectability (integrate_likelihood_extrinsic_batchmode, +26/-5). It is complementary to this PR — you fix mcsamplerPortfolio.py, #172 touches only the driver, and neither is sufficient alone:

I had independently written a plugin-discovery guard identical to your aaa8a6e1 and dropped it from #172 so the two branches don't collide. 3201b000 cherry-picks onto 4ddf41de cleanly; I verified that and ran the measurements above on the combined tree. Happy either way — squash #172 into this PR, or land them separately. If you'd rather they were one PR, say so and I'll close #172 and hand you the commit.

One thing the combined tree turned up that neither PR fixes: with both applied the portfolio gets as far as the integration loop and then aborts in the AV member's prior evaluation —

  File ".../integrators/mcsamplerAdaptiveVolume.py", line 286, in prior_prod
    p_out *= self.prior_pdf[param](x[:,indx])
TypeError: Unsupported type <class 'numpy.ndarray'>

prior_prod builds p_out on the module-global backend and multiplies in place by whatever the prior_pdf closures return. It is the same build-on-one-backend/operate-with-another shape as your fair-draw fix, one function over: standalone --sampler-method AV escapes it only because the driver rebinds mcsampler = mcsamplerAdaptiveVolume in that branch. rift_O4d already carries a fix (host-evaluate the priors, convert the product back). Flagging rather than folding it in, since it sits on the standalone-AV path and so needs its own inertness evidence on this branch — tell me if you'd rather own it.

@oshaughnessy-junior

Copy link
Copy Markdown
Author

Correction / follow-up to my note above, since I chased the prior_prod item and the answer changed.

It is not a single site, and the O4d fix is not a safe backport. Fixing each abort just revealed the next:

# site
1 mcsamplerAdaptiveVolume.prior_prod:286 AV member
2 mcsamplerEnsemble.calc_pdf:202 GMM member
3 mcsamplerPortfolio.integrate_log:471 the portfolio itself

all the same TypeError: Unsupported type <class 'numpy.ndarray'>, each only visible once the previous is fixed.

More importantly — backporting O4d's prior_prod fix verbatim regresses standalone AV, which is the shipped O4c extrinsic configuration. Measured, rho_net 51.4, seeds 9201-9204, same host/GPU:

tree result
4a8703f3 4/4 converged, fairdraw 8, eff_samp 8.00-8.72
+ O4d prior_prod 0/4, all TypeError: Unsupported type

The reason is mcsamplerGPU.uniform_samp_psi(x, xpy=xpy_default): it accepts xpy, so the fix's _prior_pdf_accepts_xpy helper passes it xpy=numpy — but the body is xpy.ones(len(x))/(cupy_pi) with cupy_pi = cupy.array(np.pi). The array honours the argument, the constant doesn't. That helper is byte-identical in O4c and O4d, so the O4d fix works upstream only in combination with the rest of O4d's backend hardening (identity_convert* sites: mcsamplerEnsemble 5 → 26, mcsamplerPortfolio 32 → 46, mcsamplerAdaptiveVolume 38 → 52; mcsamplerGPU is 43 → 47 and already carries an explicit portfolio-compatibility guard, so the AC member is fine).

So I'm not proposing any of it. I've left the reproduction on a local branch and written it up in extrinsic_collapse_demo/run/O4C_PORTFOLIO_STATUS_2026-08-13.md. My read: this PR and #172 are both worth landing on their own merits, and portfolio runnability on O4c should be scoped separately, with an inertness gate on standalone AV and standalone GMM — it is the "large lift" the original backport plan assigned to target C, and it hasn't shrunk. Regenerator for the candidate sites:

cd ~/rift_O4c_base/MonteCarloMarginalizeCode/Code/RIFT/integrators
grep -nE '(temp_ret|p_out|joint_p_s|joint_p_prior)\s*\*=' mcsampler*.py

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.

1 participant