Skip to content
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

issue 425 support per symbol dte configuration #428

Merged
merged 5 commits into from
May 26, 2024

Conversation

noamagiv
Copy link
Contributor

support DTE per symbol, same flow as target_delta.

  1. add new validation under symbol and symbol.right
  2. create get_target_dte in util (as get_target_delta)
  3. update find_eligible_contracts in portfolio_manager

test with
[symbols.XLE]
weight = 0.10
delta = 0.3
#write_threshold = 0.01
dte = 80

[symbols.BITO]
weight = 0.15
delta = 0.3

@noamagiv noamagiv closed this May 21, 2024
@noamagiv noamagiv reopened this May 21, 2024
Copy link
Owner

@brndnmtthws brndnmtthws left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good start, and I appreciate the submission.

I think we need to update/document this in thetagang.toml, then we're good to go.

It's a bit confusing that we can specify dte separately for puts/calls, but max_dte is only per-symbol, so I think that should get sorted out at some point but it can be in a separate PR.

Ideally there we be a per-symbol value (like with max_dte) which applies to both puts and calls, and then an override for each contract type that takes precedence over everything else. That will be backward-compatible, and give enough flexibility to do whatever you want.

@noamagiv
Copy link
Contributor Author

document change in toml.
you're right, DTE and max DTE behavior should be aligned, I'll change it and commit to this PR.

@noamagiv
Copy link
Contributor Author

Sorry about the mess, it was my first contribution :)

@brndnmtthws
Copy link
Owner

You need to fix the formatting issues, which you can do with the black tool. You can also install the git hooks as noted here. To run black manually, do poetry install && poetry run black .

@brndnmtthws
Copy link
Owner

You need to resolve the conflicts, which I think should be trivial, then we're good to go :shipit:

@noamagiv
Copy link
Contributor Author

You need to resolve the conflicts, which I think should be trivial, then we're good to go :shipit:

Which conflicts?

@brndnmtthws
Copy link
Owner

You need to either merge the main branch into your branch, or rebase your branch onto main. You can pull from main with git pull https://github.com/brndnmtthws/thetagang.git main.

GitHub provides two guides on how to do this if you're not familiar:

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/addressing-merge-conflicts/resolving-a-merge-conflict-on-github
https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/addressing-merge-conflicts/resolving-a-merge-conflict-using-the-command-line

@noamagiv
Copy link
Contributor Author

Thanks, I appreciate you're patience!
I've tried to rebase or merge and got this error :

noam@TATPS:~/dev/thetagang$ git pull https://github.com/brndnmtthws/thetagang.git main merge
fatal: couldn't find remote ref merge

I synced my breach with main changes from github with sync fork and it updated my PR, I hope it's ok.

It's strange, I followed the docs that you sent and I didn't saw any conflict error, just that main in ahead my branch so I just synced it.

@noamagiv noamagiv requested a review from brndnmtthws May 24, 2024 06:47
@brndnmtthws brndnmtthws merged commit a5bc7ac into brndnmtthws:main May 26, 2024
8 checks passed
@noamagiv noamagiv deleted the 425-dte-per-symbol branch May 27, 2024 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants