Add support for removing universes/entire chains via RemoveSecurity/UniverseManager.Remove#2039
Merged
Merged
Conversation
StefanoRaggi
approved these changes
May 29, 2018
| /// <returns>True if there are any differences between the two sets, false otherwise</returns> | ||
| public static bool HasDiffs<T>(this ISet<T> left, ISet<T> right) | ||
| { | ||
| return left.Except(right).Any() || right.Except(left).Any(); |
Contributor
There was a problem hiding this comment.
👍 Nice method, perhaps could be named HasDifferences or IsDifferentFrom instead ?
Contributor
Author
There was a problem hiding this comment.
agreed, there's no need for us to be short with method names :)
jaredbroad
approved these changes
May 29, 2018
| } | ||
|
|
||
| // finally, dispose and remove the canonical security from the universe manager | ||
| UniverseManager.Remove(symbol); |
Member
There was a problem hiding this comment.
this block is some cool code
| /// <param name="expiration">Option expiration date</param> | ||
| /// <returns></returns> | ||
| /// <returns>The OSI ticker representation</returns> | ||
| public static string GenerateOptionTickerOSI(string underlying, OptionRight right, decimal strikePrice, DateTime expiration) |
| { | ||
| // don't remove universe subscriptions immediately, instead mark them as disposed | ||
| // so we can turn the crank one more time to ensure we emit security changes properly | ||
| if (subscription.IsUniverseSelectionSubscription && subscription.Universe.DisposeRequested) |
Member
There was a problem hiding this comment.
Interesting... Does the security data arrive for the last crank?
Contributor
Author
There was a problem hiding this comment.
#1999 guarantees that we won't receive data after the subscription has been removed from the universe -- the final crank here is solely used to generate the security changes so that the algorithm is notified via that pathway that all of these securities have been removed.
mchandschuh
commented
May 30, 2018
| // remove child securities (option contracts for option chain universes) | ||
| foreach (var child in universe.Members.Values) | ||
| { | ||
| RemoveSecurity(child.Symbol); |
Contributor
Author
There was a problem hiding this comment.
Need to add a check before removal to ensure it's not in an other universe
mchandschuh
force-pushed
the
feature-1977-remove-universe-support
branch
from
fb01425 to
8796b48
Compare
Easy to introduce bugs when each method that is intended to do the same thing implements it internally instead of delegating to a common implementation. Also simplified syntax of invoking event handler
Useful when writing regression tests to be able to use the OSI ticker to define an expected option contract.
Implementation is simplified form of that used to detect diffs in universe selection (selection updated to use simplified form as well)
Adds the concept of universe disposal which is requested by an algorithm through invocation of UniverseManager.Remove, which is invoked via algorithm.RemoveSecurity. This instructs the data feed that the algorithm has requested to completely remove the universe and any child subscriptions from the feed. Security changes are fired for all removed securities.
No need to be so terse :)
Since we're moving towards better support of multiple universes, it's important that we check that child subscriptions aren't in those other universes before removing the security.
mchandschuh
force-pushed
the
feature-1977-remove-universe-support
branch
from
26d737d to
577c082
Compare
Martin-Molinero
added a commit
to Martin-Molinero/Lean
that referenced
this pull request
Nov 27, 2018
- Previously when a user requested to remove a universe, they would call `RemoveSecurity()`->`UniverseManager.Remove()`-> `Universe.Dispose()` -> collection change notification would call `DataManager()` that would call `RemoveSubscription()` that *would not perform any operation* since it would return `false` through `if(subscription.IsUniverseSelectionSubscription && subscription.Universe.DisposeRequested)`. Previous comment stated that the `Subscription` would be removed by the `SubscriptionSynchronizer` via `SubscriptionFinished()` but this called would go through the same if statemente as above, returning false and no performing any operation. So the `Universe` `Subscription` was never removed. PR QuantConnect#2039 - In this PR I'm removing the mentioned `if` statement and the call relation between `UniverseManager.Remove()` and `DataManager.RemoveSubscription()` since this will be covered by the `SubscriptionSynchronizer` in the next loop. - Modified regression test (originally added with these changes at QuantConnect#2039) to verify this issue.
9 tasks
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
3 participants
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
In support of #1977 we need to ensure we're able to add/remove derivative universes properly. This change implements universe removal coupled with a regression algorithm that adds and removes an option chain universe from the algorithm via
OnData.Related Issue
See #1977 (next PR will actually resolve the issue)
Motivation and Context
Aiming to support universe selection models that can add/remove
Requires Documentation Change
Yup, will want to show in documentation that you can now remove an entire option chain and all child contracts via
RemoveSecurity(optionChainSymbol)How Has This Been Tested?
Extensively tested locally as well as an extremely thorough regression test with many checks within the algorithm related to expected securities/data
Types of changes
Checklist:
bug-<issue#>-<descriptionorfeature-<issue#>-<description>