Skip to content

Fix Python input conversion for methylation-aware phasing - #1104

Open
aganezov wants to merge 1 commit into
google:r1.10from
aganezov:fix/methylation-phasing-binding
Open

aganezov wants to merge 1 commit into
google:r1.10from
aganezov:fix/methylation-phasing-binding

Conversation

@aganezov

Copy link
Copy Markdown

The methylation-aware phasing binding exposes PerformMethylationAwarePhasing directly, including its absl::Span<const Read> argument. In the tested stock runtime, Python read lists cannot be converted to that argument. Even phase([], [], []) raises TypeError before the native function executes.

This change accepts a read vector at the Python boundary and forwards it to the existing native function.

Problem and correction

The Python caller passes a list of read protobufs. The binding now exposes a lambda accepting const std::vector<Read>&, which the existing pybind11 conversion machinery supports. C++ then converts that vector to a span for the native call. The vector remains alive throughout the call, and constructing the span does not introduce another copy of its contents.

This follows the same vector-to-span adapter pattern as DirectPhasing::PhaseReadsPython. That implementation uses a named wrapper; this change keeps the short forwarding adapter inline in the binding. A named wrapper would be an equivalent alternative if preferred.

The native function signature, phasing algorithm, Python argument names, and default iteration count remain unchanged. The experimental feature remains disabled by default.

Regression coverage

Add four Python binding tests and register their test target in BUILD:

  • Empty Python lists return empty phases and p-values.
  • Reads without methylated sites retain their initial phases, including an unphased read.
  • A NumPy object array of candidate protobufs is accepted, matching the container used by the Python caller; the preset site p-value is returned correctly.
  • An empty NumPy candidate array is accepted.

These tests check input conversion and returned values. The candidate-array test uses an already-phased read and a preset p-value; it does not test calculation of a new p-value or assignment of a new phase.

Validation

The adapters were compiled using pinned runtime dependencies and exercised against the same unchanged native implementation.

Binding under test Focused regression result
Original binding All four tests fail with argument-conversion errors
Fixed binding All four tests pass

A separate local integration check ran paired ONT calling over a small real-data region with modification-tagged reads. Both runs used the same inputs and stock caller, inference, and postprocessing components, with only the binding changed. The first binding call had matching canonical input digests between runs.

The original binding raised TypeError during example generation. With the fixed binding, both native calls completed, returned one phase per read and one p-value per site, and the Python caller applied all 834 returned phases to HP tags. The pipeline completed inference and postprocessing and produced readable, indexed VCF and gVCF outputs.

No reads changed phase in this fixture, and all returned p-values were zero. Positive MI annotation writes were therefore not exercised. These results establish successful data transfer and pipeline execution; no improvement in phasing or variant-calling accuracy is claimed.

These checks used compiled adapters and existing native dependencies from a pinned runtime, not a clean Bazel build. The new BUILD target remains unverified through Bazel.

Motivation and scope

When methylation-aware phasing is enabled and the caller reaches this binding, ordinary Python read lists should be accepted. The change is limited to that adapter and focused regression coverage. It does not add a new calling mode or change when methylation-aware phasing runs.

Wrap PerformMethylationAwarePhasing in a lambda that accepts a read
vector and forwards it to the existing span-based native function.
This fixes TypeError failures when Python callers pass read lists.

Add binding regression tests for empty inputs, preservation of initial
phases without methylated sites, and NumPy candidate arrays, and
register the test target in BUILD.
@google-cla

google-cla Bot commented Sep 27, 2026

Copy link
Copy Markdown

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

This branch has not been deployed

No deployments
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