Repository navigation
Conversation
|
Hi @dcaayushd! Thank you for your pull request and welcome to our community. Action RequiredIn 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. ProcessIn 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 If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
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! |
|
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. |
Codecov Report❌ Patch coverage is 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. 🚀 New features to boost your workflow:
|
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.