Skip to content

fix(ops): make PairRolling input assert actually check its inputs#2292

Open
UditDewan wants to merge 1 commit into
microsoft:mainfrom
UditDewan:fix/pair-rolling-vacuous-assert
Open

fix(ops): make PairRolling input assert actually check its inputs#2292
UditDewan wants to merge 1 commit into
microsoft:mainfrom
UditDewan:fix/pair-rolling-vacuous-assert

Conversation

@UditDewan

Copy link
Copy Markdown

Description

The input-validation assert in PairRolling._load_internal (qlib/data/ops.py) can never fail:

assert any(
    [isinstance(self.feature_left, Expression), self.feature_right, Expression]
), "at least one of two inputs is Expression instance"

The second element is the raw feature_right value and the third is the Expression class object, which is always truthy — so any(...) is always True. The intended check was clearly "at least one of the two inputs is an Expression instance" (per the message). As a result, Cov(1.0, 2.0, 3) sails past the assert and later fails with an unrelated AttributeError: 'float' object has no attribute 'rolling'.

This PR replaces it with the intended check:

assert isinstance(self.feature_left, Expression) or isinstance(
    self.feature_right, Expression
), "at least one of two inputs is Expression instance"

Motivation and Context

Invalid PairRolling inputs (e.g. Corr/Cov built with two constants) should fail fast with the intended message instead of surfacing a confusing AttributeError deep in pandas. No valid usage changes behavior: any call with at least one Expression input passes the assert exactly as before.

How Has This Been Tested?

  • Pass the test by running: pytest qlib/tests/test_all_pipeline.py under upper directory of qlib.
  • If you are adding a new feature, test on your own test scripts.

Added tests/ops/test_pair_rolling_inputs.py (no data download needed — uses a stub Expression):

  • two non-Expression inputs now raise AssertionError (previously: passed the assert, crashed later with AttributeError)
  • Cov of two expressions still matches pandas.Series.rolling().cov()

Screenshots of Test Results (if appropriate):

  1. Pipeline test (run with MLFLOW_ALLOW_FILE_STORE=true, since mlflow 3.14 gates the ./mlruns file store behind that flag independently of this change):
tests/test_all_pipeline.py::TestAllFlow::test_0_train PASSED
tests/test_all_pipeline.py::TestAllFlow::test_1_backtest PASSED
tests/test_all_pipeline.py::TestAllFlow::test_2_expmanager PASSED
================ 3 passed, 1351 warnings in 247.62s (0:04:07) =================
  1. Your own tests:
tests/ops/test_pair_rolling_inputs.py::TestPairRollingInputCheck::test_expression_inputs_accepted PASSED
tests/ops/test_pair_rolling_inputs.py::TestPairRollingInputCheck::test_two_numeric_inputs_rejected PASSED
============================== 2 passed in 1.95s ==============================

Without the fix, test_two_numeric_inputs_rejected fails with AttributeError: 'float' object has no attribute 'rolling' at qlib/data/ops.py:1432.

Types of changes

  • Fix bugs
  • Add new feature
  • Update documentation

The assert in PairRolling._load_internal was vacuous: it evaluated
any([isinstance(feature_left, Expression), feature_right, Expression]),
where the bare Expression class object is always truthy, so the check
could never fail. Two non-Expression inputs slipped through and later
died with AttributeError ("'float' object has no attribute 'rolling'").

Check isinstance on both sides so invalid inputs fail fast with the
intended message.
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