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: trigger aligned timerule when diff is equal to period #17

Merged

Conversation

ivaaaan
Copy link
Contributor

@ivaaaan ivaaaan commented Apr 9, 2024

Aligned time rule is not triggered on timestamps like 15:00:00 with one minute rule

Copy link
Owner

@MathisWellmann MathisWellmann left a comment

Choose a reason for hiding this comment

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

Looks good overall. Some slight test modifications and we can get this merged.

Comment on lines 109 to 147
Trade {
timestamp: 1712656815000,
price: 101.0,
size: -10.0,
},
Trade {
timestamp: 1712656860000,
price: 101.0,
size: -10.0,
},
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
Trade {
timestamp: 1712656815000,
price: 101.0,
size: -10.0,
},
Trade {
timestamp: 1712656860000,
price: 101.0,
size: -10.0,
},
Trade {
timestamp: 1712656815000,
price: 100.5,
size: 10.0,
},
Trade {
timestamp: 1712656860000,
price: 101.0,
size: -10.0,
},

Would be good for both prices to be unique.

Comment on lines 136 to 137
assert_eq!(candles[0].open(), 100.00);
assert_eq!(candles[1].open(), 101.00);
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
assert_eq!(candles[0].open(), 100.00);
assert_eq!(candles[1].open(), 101.00);
assert_eq!(candles[0].open(), 100.0);
assert_eq!(candles[0].close(), 100.5);
assert_eq!(candles[1].open(), 101.0);
assert_eq!(candles[1].close(), 101.0);

An assertion for the close price would be good as well.
I think those values are correct, but unfortunate that both open and close prices are 101, so maybe it makes sense to add another trade to be included in the second candle

@ivaaaan ivaaaan force-pushed the aligned-time-rule-tick-on-0 branch from d52a61f to f4e5f12 Compare April 10, 2024 09:28
@ivaaaan
Copy link
Contributor Author

ivaaaan commented Apr 10, 2024

@MathisWellmann updated this one as well

@MathisWellmann MathisWellmann merged commit 227ade5 into MathisWellmann:main Apr 10, 2024
6 checks passed
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.

3 participants