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

Fix #1257: [EPIC, Sim] Integrate subgraph prediction into sim trading #1341

Merged

Conversation

trizin
Copy link
Contributor

@trizin trizin commented Jul 5, 2024

Fix #1257

  • includes Phase 1, by Norbert
  • Now in Phase 2, by Berkay

KatunaNorbert and others added 24 commits June 25, 2024 14:23
* added new field to ppss for type of prediction source

* add network param to sim cli and make fetch work

* verify there is enough data in database and write test

---------

Co-authored-by: Mustafa Tuncay <[email protected]>
* issue-1263: sim_engine changes

* issue-1263: part 2

* test fixes

* removed unnecessary console logs

* fix model var imps plot on chain data

* fix mypy and pylint

* issue-1263: fixes

* issue-1263: tests will be fixed at the epic branch

* change ppss config to better fit the 2 predictions

* add try catch around gql update and fix pylint

---------

Co-authored-by: Norbert <[email protected]>
* Stop tracking lake_data/ folder

* clear_test_db is added to test_get_predictions_signals_data, removed console logs

---------

Co-authored-by: Norbert <[email protected]>
@trizin trizin changed the title Use chain prediction in trading sim improvements [improvements] Use chain prediction in trading sim [improvements] Jul 5, 2024
@trentmc trentmc changed the title Use chain prediction in trading sim [improvements] Fix #1257: [EPIC, Sim] Integrate subgraph prediction into sim trading Jul 9, 2024
@trizin trizin marked this pull request as ready for review July 9, 2024 11:46
Copy link
Member

@trentmc trentmc left a comment

Choose a reason for hiding this comment

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

Much better wrt keeping sim_engine.py complexity down. A few tweaks requested, then I approve.

@@ -68,16 +68,21 @@ Copy [`ppss.yaml`](../ppss.yaml) into your own file `my_ppss.yaml` and change pa
cp ppss.yaml my_ppss.yaml
```

Chose the source of the predictions used as signals for trading:
Copy link
Member

Choose a reason for hiding this comment

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

The default to start should be not from chain.

The "optimize" for from chain should be way down in the "optimize" section.

Same for predictoor.md.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It uses built in model by default, I pushed a few improvements to further clarify that.

I don't understand, what should be changed in the "optimize" section?

Copy link
Member

Choose a reason for hiding this comment

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

Don't use the precious real estate here to tell the user that they can use on chain predictions (let alone show how). There should be no changes in this section.

Rather, much lower in the readme, it shows the user how to use on chain predictions.

test_n: Optional[int] = None,
tradetype: Optional[str] = None,
) -> dict:
d = {
"log_dir": log_dir,
"use_own_model": use_own_model,
Copy link
Member

Choose a reason for hiding this comment

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

"use_own_model" needs to get added to unit tests, following the same pattern as other variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't quite understand that. use_own_model is set to True by default, so no changes are required in the test files.

if mergedohlcv_df is None:
f = OhlcvDataFactory(self.ppss.lake_ss)
mergedohlcv_df = f.get_mergedohlcv_df()

# fetch predictions data
Copy link
Member

Choose a reason for hiding this comment

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

This should be part of _init_loop_attributes()

def load_chain_prediction_data(self):
SimChainPredictions.verify_use_chain_data_in_syms_dependencies(self.ppss)
if not SimChainPredictions.verify_prediction_data(self.ppss):
raise Exception(
Copy link
Member

Choose a reason for hiding this comment

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

Make the error text shorter so that only one line is used, not three.

prob_up = self.model.predict_ptrue(X_test)[0] # in [0.0, 1.0]
else:
ut_seconds = ut.to_seconds()
prediction = self.chain_predictions_map.get(ut_seconds)
Copy link
Member

Choose a reason for hiding this comment

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

Rename "prediction" to "prob_up". Then you can delete two lines below.

@trizin trizin requested a review from trentmc July 17, 2024 13:04
@trizin trizin merged commit 7a4b845 into main Jul 18, 2024
5 checks passed
@trizin trizin deleted the issue-1257-use-chain-prediction-in-trading-sim-improvements branch July 18, 2024 16:30
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.

[EPIC, Sim] Integrate subgraph prediction into sim trading
4 participants