Fix get_max_profit/get_max_loss for single-leg strategies by ms32035 · Pull Request #284 · joaquinbejar/OptionStratLib

ms32035 · 2026-03-10T08:25:19Z

All four single-leg strategies (LongCall, LongPut, ShortCall, ShortPut) evaluated profit/loss at the strike price, which is incorrect:

LongCall at strike always yields -premium (worthless call)
LongPut at strike always yields -premium (worthless put)
ShortCall returned Err for unlimited loss instead of Positive::INFINITY
ShortPut at strike yields +premium, failing the loss check

Fixed by using correct evaluation points and Positive::INFINITY for unlimited profit/loss, consistent with how LongStraddle handles it.

All four single-leg strategies (LongCall, LongPut, ShortCall, ShortPut) evaluated profit/loss at the strike price, which is incorrect: - LongCall at strike always yields -premium (worthless call) - LongPut at strike always yields -premium (worthless put) - ShortCall returned Err for unlimited loss instead of Positive::INFINITY - ShortPut at strike yields +premium, failing the loss check Fixed by using correct evaluation points and Positive::INFINITY for unlimited profit/loss, consistent with how LongStraddle handles it.

Copilot

Pull request overview

Fixes incorrect max profit/loss calculations for single-leg option strategies by evaluating at the correct underlying prices and representing unbounded outcomes as Positive::INFINITY.

Changes:

Correct max profit/loss evaluation points for LongPut and ShortPut (e.g., price = 0 where appropriate).
Represent unbounded max profit/loss for LongCall and ShortCall as Positive::INFINITY instead of erroring.
Update ShortCall unit tests to reflect the new INFINITY behavior and near-zero profit ratio.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
tests/unit/strategies/short_call_test.rs	Updates expectations for `ShortCall` max loss (now `INFINITY`) and profit ratio behavior.
src/strategies/short_put.rs	Fixes `ShortPut` max profit/loss logic using correct evaluation points (loss at underlying = 0).
src/strategies/short_call.rs	Fixes `ShortCall` max profit (net premium) and returns `INFINITY` for unlimited max loss.
src/strategies/long_put.rs	Fixes `LongPut` max profit to evaluate at underlying = 0; max loss becomes total cost.
src/strategies/long_call.rs	Fixes `LongCall` max profit to `INFINITY`; max loss becomes total cost.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

tests/unit/strategies/short_call_test.rs

Copilot · 2026-03-10T14:49:48Z

src/strategies/long_call.rs

    fn get_max_profit(&self) -> Result<Positive, StrategyError> {
-        let profit = self.calculate_profit_at(&self.long_call.option.strike_price)?;
-        if profit >= Decimal::ZERO {
-            Ok(Positive::new_decimal(profit)?)
-        } else {
-            Err(StrategyError::ProfitLossError(
-                ProfitLossErrorKind::MaxProfitError {
-                    reason: "Net premium received is negative".to_string(),
-                },
-            ))
-        }
+        Ok(Positive::INFINITY) // Theoretically unlimited
    }
    fn get_max_loss(&self) -> Result<Positive, StrategyError> {
-        let loss = self.calculate_profit_at(&self.long_call.option.strike_price)?;
-        if loss <= Decimal::ZERO {
-            Ok(Positive::new_decimal(loss.abs()).unwrap_or(Positive::ZERO))
-        } else {
-            Err(StrategyError::ProfitLossError(
-                ProfitLossErrorKind::MaxLossError {
-                    reason: "Max loss is negative".to_string(),
-                },
-            ))
-        }
+        // Max loss for a long call is the premium paid (at any price ≤ strike).


Returning Positive::INFINITY for max profit interacts poorly with the existing get_profit_area/get_profit_ratio implementations: they convert via to_f64() and then Decimal::from_f64(...), which returns None for Inf/overflow and gets coerced to Decimal::ZERO via unwrap_or. That makes metrics like profit_ratio/area silently become 0 for an unlimited-profit strategy. Add explicit handling for Positive::INFINITY (e.g., return Decimal::MAX or a documented sentinel) instead of relying on float/Decimal conversions.

marty-mcclaw

Technical fix for max profit/loss calculations in single-leg strategies. Moving to Positive::INFINITY for unlimited exposure is much more consistent with the rest of the library's payoff modeling. A few observations on the ShortPut evaluation and test assertions.

src/strategies/short_put.rs

src/strategies/long_call.rs

tests/unit/strategies/short_call_test.rs

codecov · 2026-03-10T14:52:34Z

Codecov Report

❌ Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/strategies/long_put.rs	66.66%	1 Missing ⚠️

Files with missing lines	Coverage Δ
src/strategies/long_call.rs	`71.20% <100.00%> (+1.26%)`	⬆️
src/strategies/short_call.rs	`74.31% <100.00%> (+0.95%)`	⬆️
src/strategies/short_put.rs	`72.67% <100.00%> (+0.49%)`	⬆️
src/strategies/long_put.rs	`71.96% <66.66%> (+0.79%)`	⬆️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

joaquinbejar

Thanks for the PR @ms32035! The bug analysis is correct — all four single-leg strategies were evaluating profit/loss at the strike price, which for ATM options always yields -premium (worthless option).

Overall Assessment

The mathematical reasoning is sound and consistent with how LongStraddle/ShortStraddle/ShortStrangle already handle unlimited profit/loss via Positive::INFINITY.

CI Failure

The GitHub Actions pipeline reported a lint failure. I recommend running the following before pushing:

make lint-fix pre-push

This will fix formatting issues and ensure clippy, tests, and build all pass cleanly.

Suggestions

I have left a few inline comments. The main points:

get_profit_ratio / get_profit_area silently return Decimal::ZERO when max_profit or max_loss is Positive::INFINITY — this is a pre-existing issue not introduced by this PR, but now more visible. Consider handling the INFINITY case explicitly in get_profit_ratio (returning Decimal::MAX or Decimal::ZERO depending on which side is infinite). I can open a separate issue for this.
Test coverage: the existing tests for long_call, long_put, and short_put use flexible match Ok/Err patterns that still pass, but it would be good to tighten them to explicitly assert the new correct behavior (similar to what you did for short_call_test.rs).
Consider adding missing tests for long_call_test, long_put_test, and short_put_test that explicitly verify Positive::INFINITY and get_total_cost() results.

joaquinbejar · 2026-03-10T14:57:25Z

src/strategies/long_call.rs

-                },
-            ))
-        }
+        Ok(Positive::INFINITY) // Theoretically unlimited


✅ Correct: Long Call has theoretically unlimited upside. Consistent with LongStraddle and LongStrangle patterns.

⚠️ Note: since get_profit_ratio uses Decimal::from_f64(max_profit.to_f64() / max_loss.to_f64() * 100.0).unwrap_or(Decimal::ZERO), when max_profit = INFINITY, this computes INFINITY / cost * 100 = INFINITY, and Decimal::from_f64(INFINITY) returns None, so get_profit_ratio silently returns Decimal::ZERO. This is a pre-existing issue (before the fix, the error path caused the same result), but worth noting for a follow-up.

src/strategies/long_call.rs

joaquinbejar · 2026-03-10T14:57:26Z

src/strategies/long_put.rs

-        let profit = self.calculate_profit_at(&self.long_put.option.strike_price)?;
+        // Max profit for a long put occurs at price = 0: strike - premium.
+        let profit = self.calculate_profit_at(&Positive::ZERO)?;
        if profit >= Decimal::ZERO {


✅ Correct: evaluating at Positive::ZERO gives the maximum profit for a long put (strike - premium - fees). This is the right evaluation point.

src/strategies/long_put.rs

joaquinbejar · 2026-03-10T14:57:26Z

src/strategies/short_call.rs

-                },
-            ))
-        }
+        // Max profit for a short call is the net premium received (at any price ≤ strike).


✅ get_net_premium_received() is the right abstraction for the max profit of a short option. Produces the same numerical result as the previous calculate_profit_at(strike) but is semantically clearer.

joaquinbejar · 2026-03-10T14:57:26Z

src/strategies/short_call.rs

+        // Max profit for a short call is the net premium received (at any price ≤ strike).
+        self.get_net_premium_received()
    }
    fn get_max_loss(&self) -> Result<Positive, StrategyError> {


✅ Returning Positive::INFINITY instead of Err is the correct pattern. This is consistent with ShortStraddle, ShortStrangle, and CallButterfly. The previous Err approach caused get_profit_ratio to use unwrap_or(Positive::ZERO) and then hit the (_, ZERO) branch returning Decimal::MAX, which was misleading.

src/strategies/short_put.rs

joaquinbejar · 2026-03-10T14:57:26Z

tests/unit/strategies/short_call_test.rs

+    // Max loss for a short call is theoretically unlimited (Positive::INFINITY).
+    assert!(result.is_ok());
+    assert_eq!(result.unwrap(), Positive::INFINITY);
 }


✅ Good update. The old test was asserting that unlimited loss was an Err, which was the root cause of the semantic issue. Now correctly checks for Positive::INFINITY.

💡 Suggestion: consider adding similar explicit assertions in long_call_test.rs (assert get_max_profit() == Positive::INFINITY), long_put_test.rs (assert get_max_loss() == get_total_cost()), and short_put_test.rs (assert get_max_loss() matches calculate_profit_at(ZERO).abs()). The existing tests use flexible match Ok/Err patterns that still pass, but explicit assertions would be stronger.

joaquinbejar · 2026-03-10T14:57:26Z

tests/unit/strategies/short_call_test.rs

-    assert_eq!(ratio_result.unwrap(), Decimal::MAX); // Based on current ShortCall impl
+    let ratio = ratio_result.unwrap();
+    assert!(
+        ratio < Decimal::new(1, 10),


✅ Good fix. With max_loss = Positive::INFINITY, the ratio max_profit / INFINITY * 100 ≈ 0, which is mathematically correct for a strategy with unlimited downside.

The assertion ratio < Decimal::new(1, 10) (i.e. < 1e-10) is appropriate since the actual value will be exactly 0.0.

joaquinbejar · 2026-03-10T15:00:49Z

⚠️ CI Lint Failure

The GitHub Actions pipeline is reporting a lint failure on this PR. Before pushing updated commits, please run:

make lint-fix pre-push

This ensures:

cargo fmt formatting is applied
cargo clippy -- -D warnings passes with zero warnings
All tests pass
Release build compiles cleanly

This is a requirement for merging.

joaquinbejar requested review from Copilot and joaquinbejar March 10, 2026 14:43

Copilot started reviewing on behalf of joaquinbejar March 10, 2026 14:43 View session

Copilot AI reviewed Mar 10, 2026

View reviewed changes

marty-mcclaw reviewed Mar 10, 2026

View reviewed changes

src/strategies/short_put.rs Show resolved Hide resolved

src/strategies/long_call.rs Show resolved Hide resolved

tests/unit/strategies/short_call_test.rs Show resolved Hide resolved

joaquinbejar reviewed Mar 10, 2026

View reviewed changes

apply review fixes

e71b083

Conversation

ms32035 commented Mar 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

marty-mcclaw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

joaquinbejar left a comment

Choose a reason for hiding this comment

Overall Assessment

CI Failure

Suggestions

Uh oh!

joaquinbejar Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joaquinbejar Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joaquinbejar Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

joaquinbejar Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

joaquinbejar Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

joaquinbejar Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

joaquinbejar commented Mar 10, 2026

⚠️ CI Lint Failure

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Mar 10, 2026 •

edited

Loading