fix(ops): make PairRolling input assert actually check its inputs#2292
Open
UditDewan wants to merge 1 commit into
Open
fix(ops): make PairRolling input assert actually check its inputs#2292UditDewan wants to merge 1 commit into
UditDewan wants to merge 1 commit into
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The input-validation assert in
PairRolling._load_internal(qlib/data/ops.py) can never fail:The second element is the raw
feature_rightvalue and the third is theExpressionclass object, which is always truthy — soany(...)is alwaysTrue. The intended check was clearly "at least one of the two inputs is anExpressioninstance" (per the message). As a result,Cov(1.0, 2.0, 3)sails past the assert and later fails with an unrelatedAttributeError: 'float' object has no attribute 'rolling'.This PR replaces it with the intended check:
Motivation and Context
Invalid
PairRollinginputs (e.g.Corr/Covbuilt with two constants) should fail fast with the intended message instead of surfacing a confusingAttributeErrordeep in pandas. No valid usage changes behavior: any call with at least oneExpressioninput passes the assert exactly as before.How Has This Been Tested?
pytest qlib/tests/test_all_pipeline.pyunder upper directory ofqlib.Added
tests/ops/test_pair_rolling_inputs.py(no data download needed — uses a stubExpression):AssertionError(previously: passed the assert, crashed later withAttributeError)Covof two expressions still matchespandas.Series.rolling().cov()Screenshots of Test Results (if appropriate):
MLFLOW_ALLOW_FILE_STORE=true, since mlflow 3.14 gates the./mlrunsfile store behind that flag independently of this change):Without the fix,
test_two_numeric_inputs_rejectedfails withAttributeError: 'float' object has no attribute 'rolling'atqlib/data/ops.py:1432.Types of changes