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

Implement high-precision 128-bit value types #2072

Merged
merged 99 commits into from
Jan 15, 2025
Merged

Conversation

twitu
Copy link
Collaborator

@twitu twitu commented Nov 24, 2024

Pull Request

  • Use 128-bit integers to represent higher precisions for value types.
  • Use high-precision feature flag to switch between lower and higher precision.

See RFC.

@cjdsellers cjdsellers changed the title Higher precision with i128 for raw price High-precision 128-bit value types Dec 31, 2024
@@ -67,8 +73,10 @@ PRICE_MIN = RUST_PRICE_MIN
MONEY_MAX = RUST_MONEY_MAX
MONEY_MIN = RUST_MONEY_MIN

FIXED_PRECISION = RUST_FIXED_PRECISION
FIXED_SCALAR = RUST_FIXED_SCALAR
HIGH_PRECISION = PRECISION == 16
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It'll be a safer option to load this from the cdef HIGH_PRECISION value defined in model.pxd. Just in case the digits of precision change for high precision it'll very difficult to track down why this is failing.

Copy link
Member

@cjdsellers cjdsellers Jan 15, 2025

Choose a reason for hiding this comment

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

Also had a similar thought, had to add another constant generated at compile time, as HIGH_PRECISION was already occupied with the DEF, but now achieving what we were after:

HIGH_PRECISION = RUST_HIGH_PRECISION_MODE

@cjdsellers cjdsellers changed the title High-precision 128-bit value types Implement high-precision 128-bit value types Jan 14, 2025
@twitu twitu marked this pull request as ready for review January 15, 2025 03:09
Copy link

codecov bot commented Jan 15, 2025

Codecov Report

Attention: Patch coverage is 77.77778% with 58 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
nautilus_trader/persistence/wranglers_v2.py 44.44% 35 Missing ⚠️
nautilus_trader/model/data.pyx 80.00% 14 Missing ⚠️
nautilus_trader/persistence/wranglers.pyx 90.90% 5 Missing ⚠️
nautilus_trader/model/objects.pxd 75.00% 2 Missing ⚠️
nautilus_trader/model/objects.pyx 94.44% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

@@ -191,8 +186,9 @@ mod tests {
types::{Price, Quantity},
};

#[cfg(feature = "high-precision")] // TODO: Add 64-bit precision version of test
Copy link
Member

Choose a reason for hiding this comment

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

We'll do another sweep of the tests once merged.

);
// TODO: Revisit calculations
Copy link
Member

Choose a reason for hiding this comment

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

This was outputting an unscaled raw quantity, we probably want the scaled quantity.

@@ -15,7 +15,7 @@

//! Represents a medium of exchange in a specified denomination with a fixed decimal precision.
//!
//! Handles up to 9 decimals of precision.
//! Handles up to 16 decimals of precision.
Copy link
Member

Choose a reason for hiding this comment

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

Will need to tidy up the docs some more depending on precision mode.

@@ -1201,13 +1200,13 @@ class PyCondition:
Condition.positive(value, param, ex_type)

@staticmethod
def positive_int(int64_t value, str param, ex_type = None):
def positive_int(value: int, str param, ex_type = None):
Copy link
Member

Choose a reason for hiding this comment

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

Relaxed all of these C types, as they're coming from Python in non-performance critical function calls.
Avoids having to name the specific integer types in C.

@filipmacek filipmacek self-requested a review January 15, 2025 10:43
@cjdsellers cjdsellers merged commit d51f0fd into develop Jan 15, 2025
13 checks passed
@cjdsellers cjdsellers deleted the high-precision branch January 15, 2025 22:56
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