nautilus_trader icon indicating copy to clipboard operation
nautilus_trader copied to clipboard

add get_avg_px_qty_for_exposure rust function with test

Open elementace opened this issue 1 year ago • 4 comments

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.

elementace avatar Aug 30 '24 07:08 elementace

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Aug 30 '24 07:08 CLAassistant

Looks good to me. But I would like to see some not-zero test cases, with simple numbers, just so that we are sure

filipmacek avatar Aug 30 '24 08:08 filipmacek

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

cjdsellers avatar Aug 31 '24 21:08 cjdsellers

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.

cjdsellers avatar Sep 24 '24 04:09 cjdsellers

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:

cjdsellers avatar Oct 23 '24 06:10 cjdsellers

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!

cjdsellers avatar Oct 27 '24 06:10 cjdsellers