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

add get_avg_px_qty_for_exposure rust function with test #1893

Open
wants to merge 26 commits into
base: develop
Choose a base branch
from

Conversation

elementace
Copy link

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

  • 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.

See release notes.
See release notes.
See release notes.
See release notes.
See release notes.
See release notes.
See release notes.
See release notes.
See release notes.
See release notes.
See release notes.
See release notes.
See release notes.
See release notes.
See release notes.
See release notes.
See release notes.
See release notes.
See release notes.
See release notes.
See release notes.
See release notes.
See release notes.
See release notes.
@CLAassistant
Copy link

CLAassistant commented Aug 30, 2024

CLA assistant check
All committers have signed the CLA.

@filipmacek
Copy link
Member

filipmacek commented Aug 30, 2024

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

@cjdsellers
Copy link
Member

cjdsellers commented Aug 31, 2024

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

@elementace elementace force-pushed the feature/get_avg_px_qty_for_exposure branch from a52059f to 7ef467f Compare September 9, 2024 06:34
@cjdsellers cjdsellers changed the base branch from master to develop September 9, 2024 06:43
@cjdsellers
Copy link
Member

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.

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.

5 participants