-
Notifications
You must be signed in to change notification settings - Fork 0
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
48 feature eval computation #55
base: main
Are you sure you want to change the base?
Conversation
questions:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why r we using dateutil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im stupid, removed and iterating with regular datetime
src/pysrc/util/feature_eval.py
Outdated
self.asset = asset | ||
self.start = start | ||
self.end = end | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
del
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/pysrc/util/feature_eval.py
Outdated
|
||
|
||
class Evaluator: | ||
def __init__(self, features: list[str], asset: str, start: datetime, end: datetime): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
asset enum
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/pysrc/util/feature_eval.py
Outdated
trade_generators = [] | ||
snapshot_generators = [] | ||
|
||
for date in rrule(DAILY, dtstart=start, until=end): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we do all the path manip with pathlib
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, done
src/pysrc/util/feature_eval.py
Outdated
feature_dict[feature] = [] | ||
|
||
for trade, snapshot in data: | ||
print("huh") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bro
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😭
src/pysrc/util/feature_eval.py
Outdated
|
||
class Evaluator: | ||
def __init__(self, features: list[str], asset: str, start: datetime, end: datetime): | ||
self.features = features |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefix with underscore pls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefix the helpers? are you talking about calculate_features()
I thought it wasn't a helper, but would it be better to combine calculate_features and evaluate_features into one "calc_and_eval_features" method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mb i meant like self._features = features
src/pysrc/util/feature_eval.py
Outdated
self, | ||
start: datetime, | ||
end: datetime, | ||
trades_resource_path: str, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we should pass resource_path
as a class parameter, and then we can do resouce_path
/trades/asset/date.bin here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/pysrc/util/feature_eval.py
Outdated
trade_generators = [] | ||
snapshot_generators = [] | ||
|
||
for date in rrule(DAILY, dtstart=start, until=end): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not anything wrong with using but was there a reason you used rrule instead of doing this manually
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i was dumb
src/pysrc/util/feature_eval.py
Outdated
for feature in self.features: | ||
feature_dict[feature] = [] | ||
|
||
for trade, snapshot in data: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uhhhh i think this is assuming that trades/snapshots start at the same tick and have an entry for every tick which i don't think is guaranteed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea that is not guaranteed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may need to mute to use jerrys pr
not sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was alr added by chris in #66
Description
Testing
Impact
Other