fix(portfolio): handle NaN trade plot markers - #869
Open
JohnDReynolds wants to merge 1 commit into
Open
Conversation
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.
Fixes #716.
Summary
Prevent non-finite open-trade returns from producing invalid Plotly marker metadata in trade-PnL plots.
Finite returns retain their existing relative marker scaling. Trades with non-finite returns receive the lower marker-size and opacity bounds, while their underlying PnL and return values remain unchanged.
Problem
A portfolio with an open trade whose final valuation is unknown can produce a non-finite trade return:
On master, this raises a Plotly
ValueErrorbecauseNaNis passed as marker opacity.Trades.plot_pnl()previously scaled all absolute returns together. A single non-finite return therefore contaminated the minimum and maximum used to calculate marker sizes and opacities.Change
Trades.plot_pnl()usage;Portfolio.plot_trade_pnl()usage;Independent numerical check
For two long trades moving from
1 -> 2and1 -> 3, the independently calculated returns are[1.0, 2.0].They continue to map to:
[7.0, 14.0];[0.75, 0.9].The non-finite open trade receives the lower bounds:
7.0;0.75.Grouped direct, delegated, and
Portfolio.plot()orchestrated paths were also checked with physical-column selection.Verification
1 passed, 84 deselectedtests/test_plotting.py:85 passed941 passed, 100 skippedgit diff --check: passedThe changed-file Black check reports that both files would be reformatted, matching the unchanged upstream baseline. No unrelated formatting changes are included.
Engine and scope implications
This change affects only Python/NumPy/Plotly presentation logic after trade records have been generated.
It does not change: