-
Notifications
You must be signed in to change notification settings - Fork 34
Fix get_max_profit/get_max_loss for single-leg strategies #284
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,6 @@ | ||
| use super::base::{BreakEvenable, Positionable, StrategyType}; | ||
| use crate::backtesting::results::{SimulationResult, SimulationStatsResult}; | ||
| use crate::chains::OptionChain; | ||
| use crate::error::strategies::ProfitLossErrorKind; | ||
| use crate::error::{ | ||
| GreeksError, PricingError, ProbabilityError, SimulationError, StrategyError, | ||
| position::{PositionError, PositionValidationErrorKind}, | ||
|
|
@@ -277,28 +276,11 @@ impl BreakEvenable for LongCall { | |
|
|
||
| impl Strategies for LongCall { | ||
| 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 | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ Correct: Long Call has theoretically unlimited upside. Consistent with
|
||
| } | ||
| 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). | ||
|
Comment on lines
278
to
+282
|
||
| Ok(self.get_total_cost()?) | ||
| } | ||
| fn get_profit_area(&self) -> Result<Decimal, StrategyError> { | ||
| let high = self.get_max_profit().unwrap_or(Positive::ZERO); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -274,28 +274,21 @@ impl BreakEvenable for LongPut { | |
|
|
||
| impl Strategies for LongPut { | ||
| fn get_max_profit(&self) -> Result<Positive, StrategyError> { | ||
| 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 { | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ Correct: evaluating at |
||
| Ok(Positive::new_decimal(profit)?) | ||
| } else { | ||
| Err(StrategyError::ProfitLossError( | ||
| ProfitLossErrorKind::MaxProfitError { | ||
| reason: "Net premium received is negative".to_string(), | ||
| reason: "Max profit is negative".to_string(), | ||
| }, | ||
| )) | ||
| } | ||
| } | ||
| fn get_max_loss(&self) -> Result<Positive, StrategyError> { | ||
| let loss = self.calculate_profit_at(&self.long_put.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 put is the premium paid (at any price ≥ strike). | ||
|
ms32035 marked this conversation as resolved.
|
||
| Ok(self.get_total_cost()?) | ||
| } | ||
| fn get_profit_area(&self) -> Result<Decimal, StrategyError> { | ||
| let high = self.get_max_profit().unwrap_or(Positive::ZERO); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,6 @@ use super::base::{BreakEvenable, Positionable, StrategyType}; | |
| use crate::backtesting::results::{SimulationResult, SimulationStatsResult}; | ||
|
|
||
| use crate::chains::OptionChain; | ||
| use crate::error::strategies::ProfitLossErrorKind; | ||
| use crate::error::{ | ||
| GreeksError, PricingError, ProbabilityError, SimulationError, StrategyError, | ||
| position::{PositionError, PositionValidationErrorKind}, | ||
|
|
@@ -289,24 +288,11 @@ impl BreakEvenable for ShortCall { | |
|
|
||
| impl Strategies for ShortCall { | ||
| fn get_max_profit(&self) -> Result<Positive, StrategyError> { | ||
| let profit = self.calculate_profit_at(&self.short_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(), | ||
| }, | ||
| )) | ||
| } | ||
| // Max profit for a short call is the net premium received (at any price ≤ strike). | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ |
||
| self.get_net_premium_received() | ||
| } | ||
| fn get_max_loss(&self) -> Result<Positive, StrategyError> { | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ Returning |
||
| // Max loss for a short call is theoretically unlimited. | ||
| Err(StrategyError::ProfitLossError( | ||
| ProfitLossErrorKind::MaxLossError { | ||
| reason: "Maximum loss is unlimited for a short call.".to_string(), | ||
| }, | ||
| )) | ||
| Ok(Positive::INFINITY) // Theoretically unlimited | ||
| } | ||
| fn get_profit_area(&self) -> Result<Decimal, StrategyError> { | ||
| let high = self.get_max_profit().unwrap_or(Positive::ZERO); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,6 @@ | ||
| use chrono::Utc; | ||
| use optionstratlib::{ | ||
| ExpirationDate, Options, | ||
| error::StrategyError, | ||
| error::strategies::ProfitLossErrorKind, | ||
| model::{ | ||
| position::Position, | ||
| types::{OptionStyle, OptionType, Side}, | ||
|
|
@@ -164,16 +162,9 @@ fn test_short_call_get_max_profit() { | |
| fn test_short_call_get_max_loss() { | ||
| let short_call = create_test_short_call(); | ||
| let result = short_call.get_max_loss(); | ||
| assert!(result.is_err()); | ||
| match result.err().unwrap() { | ||
| StrategyError::ProfitLossError(kind) => match kind { | ||
| ProfitLossErrorKind::MaxLossError { reason } => { | ||
| assert!(reason.to_lowercase().contains("unlimited")); | ||
| } | ||
| _ => panic!("Expected MaxLossError, got {kind:?}"), | ||
| }, | ||
| e => panic!("Expected ProfitLossError, got {e:?}"), | ||
| } | ||
| // Max loss for a short call is theoretically unlimited (Positive::INFINITY). | ||
| assert!(result.is_ok()); | ||
| assert_eq!(result.unwrap(), Positive::INFINITY); | ||
| } | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ Good update. The old test was asserting that unlimited loss was an 💡 Suggestion: consider adding similar explicit assertions in |
||
|
|
||
| #[test] | ||
|
|
@@ -300,12 +291,11 @@ fn test_short_call_get_positions() { | |
| fn test_short_call_get_profit_ratio() { | ||
| let short_call = create_test_short_call(); | ||
| let ratio_result = short_call.get_profit_ratio(); | ||
| // For a short call, max loss is unlimited, so profit ratio should be Decimal::MAX (or an error if not handled gracefully) | ||
| // The current implementation returns Decimal::MAX if max_loss is zero, which is not the case here. | ||
| // If max_profit > 0 and max_loss is effectively infinite (represented by an error), then the ratio is effectively zero or very small. | ||
| // However, the current `short_call.get_max_loss()` returns an error because loss is unlimited. | ||
| // The `get_profit_ratio` in `short_call.rs` handles this by returning Decimal::MAX if `get_max_loss` is an error (interpreted as max_loss -> infinity, so profit/infinity -> 0, but the code has it as Decimal::MAX) | ||
| // Let's adjust the expectation based on the `ShortCall` implementation of `get_profit_ratio`. | ||
| // Max loss is Positive::INFINITY, so profit_ratio = max_profit / INFINITY * 100 ≈ 0. | ||
| assert!(ratio_result.is_ok()); | ||
| assert_eq!(ratio_result.unwrap(), Decimal::MAX); // Based on current ShortCall impl | ||
| let ratio = ratio_result.unwrap(); | ||
| assert!( | ||
|
ms32035 marked this conversation as resolved.
|
||
| ratio < Decimal::new(1, 1), | ||
| "Expected near-zero ratio, got {ratio}" | ||
| ); | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.