Skip to content

Fix read-only observation arrays in transforms - #5306

Closed
dcaayushd wants to merge 1 commit into
meta:mainfrom
dcaayushd:fix/read-only-observation-arrays
Closed

dcaayushd wants to merge 1 commit into
meta:mainfrom
dcaayushd:fix/read-only-observation-arrays

Conversation

@dcaayushd

Copy link
Copy Markdown

Fixes #4879

Problem

cross_validate and CrossValidationPlot can fail with:
ValueError: assignment destination is read-only

This occurs when pandas provides read-only NumPy arrays for ObservationData.means or ObservationData.covariance. Several Ax transforms mutate these arrays in place, including Winsorize.

Cause

Transforms such as Winsorize assume that observation arrays are writable:
obsd.means[idx] = ...

With pandas 3.0, arrays extracted from DataFrames may be read-only by default.

Fix

Copy means and covariance only when they are read-only before performing in-place mutations. This preserves existing behavior for writable arrays while allowing transforms to safely handle read-only inputs.

The fix covers:

Winsorize
LogY
PowerTransformY
StandardizeY
BilogY

Testing

Added a regression test that passes read-only means and covariance arrays through Winsorize.transform_observations and verifies the expected winsorized values.

Also verified that the original CrossValidationPlot reproduction succeeds on the current branch.

@meta-cla

meta-cla Bot commented Oct 21, 2026

Copy link
Copy Markdown

Hi @dcaayushd!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

@meta-cla

meta-cla Bot commented Oct 21, 2026

Copy link
Copy Markdown

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@meta-cla meta-cla Bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Oct 21, 2026
@dcaayushd

Copy link
Copy Markdown
Author

Hi @saitcakmak, thanks for maintaining Ax! This PR fixes #4879 (CrossValidationPlot failing with "assignment destination is read-only" in Winsorize).

When you have a moment, could you take a look? I'm happy to make any changes you'd like. Thanks!

@saitcakmak

Copy link
Copy Markdown
Contributor

Hi @dcaayushd. Thanks for sending the PR. Looks like the underlying bug for #4879 was already resolved by #4814, which was included in Ax 1.2.3 and newer versions.

A note on the approach on this PR: Checking for the arrays being writable in multiple places is a bit of a brute force solution. This PR already misses another transform that would've had the same bug (stratified standardize), and the fix wouldn't apply to any new transform we add. A better fix would be to ensure these arrays are writeable in a shared location, e.g., where the cross validation code creates ObservationData. That's the path #4814 takes, by constructing ObservationData with writeable arrays.

@saitcakmak saitcakmak closed this Oct 21, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 67.74194% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.59%. Comparing base (80694ce) to head (8b809d5).

Files with missing lines Patch % Lines
ax/adapter/transforms/power_transform_y.py 50.00% 4 Missing ⚠️
ax/adapter/transforms/bilog_y.py 50.00% 2 Missing ⚠️
ax/adapter/transforms/log_y.py 50.00% 2 Missing ⚠️
ax/adapter/transforms/standardize_y.py 50.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5306      +/-   ##
==========================================
- Coverage   96.60%   96.59%   -0.02%     
==========================================
  Files         621      621              
  Lines       71085    71116      +31     
==========================================
+ Hits        68674    68695      +21     
- Misses       2411     2421      +10     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed Do not delete this pull request or issue due to inactivity.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: CrossValidation is broken, fails with ValueError

3 participants