nautilus_trader
nautilus_trader copied to clipboard
add get_avg_px_qty_for_exposure rust function with test
Pull Request
Added get_avg_px_qty_for_exposure rust function for Order books rust object
get_avg_px_for_quantity Currently exists, to estimate the average price for a
required quantity in an Order book side.;
Often traders want to know the level they need to pay for a set 'exposure' or 'notional' amount.
This value is different to get_avg_px_for_quantity. Furthermore, it will provide both a price and a qty,
as opposed to just a price.
I am unclear on how best to implement into the c headers, as it returns a tuple of doubles; It might require a new struct object; I'll leave that design decision up to @cjdsellers
Type of change
- [X] New feature (non-breaking change which adds functionality)
How has this change been tested?
I've added a basic 0 test, but not a specific value test as I don't have a full test setup.
Looks good to me. But I would like to see some not-zero test cases, with simple numbers, just so that we are sure
Hey @elementace
Thanks for the contribution!
You're correct that we'd need a struct to return the two doubles in C (I don't think the alternative of CVec or pointers is a good idea) under the hood in Cython. Then we can return a tuple of two doubles up to Python using tuple[double, double]. I can handle this though as there's some hurdles on the cbindgen and Cython side.
So I can merge this in if we just cover off two things:
- Confirming you've signed the CLA? (I think there's an issue on their end where the badge doesn't update even after signing)
- I agree with @filipmacek that we probably need a couple more non-zero value tests, which also aids understanding the calculation too
Hi @elementace
Looks like we'll need a struct after all:
error: `extern` fn uses type `(f64, f64, f64)`, which is not FFI-safe
Error: --> model/src/ffi/orderbook/book.rs:249:6
|
249 | ) -> (f64, f64, f64) {
| ^^^^^^^^^^^^^^^ not FFI-safe
|
= help: consider using a struct instead
= note: tuples have unspecified layout
= note: `-D improper-ctypes-definitions` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(improper_ctypes_definitions)]`
As above, I'm happy to take this part on if you just add a couple of Rust tests with non-zero values.
Hey @elementace
If you're able to comment out the FFI functions and run pre-commit, then we can merge this and I'll take it the rest of the way :+1:
Hi @elementace
I've merged in most of your changes with commit c105b7e256bff3eb08c7d87a0d2fd1ded8c8263b, just leaving the FFI behind for now.
I'll get back to this once the Rust port is done and we're onto adding order book features, otherwise feel free to pick this up if you need the exposure through Cython before then.
Many thanks once again!