Skip to content

Interested in collaborating #48

@jonathanstrong

Description

@jonathanstrong

Hello,

Earlier this evening I randomly came across your post on users.rust-lang.org from several months ago looking for feedback on this library. I wasn't previously aware of the project and was impressed by its breadth and the amount of work that's already gone into it. I run a trading operation on a rust codebase so this is definitely of major interest to me! I'd be interested in collaborating and wanted to send some initial thoughts from a couple hours with the code.

You mentioned in the post you were new to rust, so I included various recommendations from my own experience. Perhaps some of it will be obvious or things you already know - but I thought it couldn't hurt. Like you, I see rust as an incredible tool for this application, owing to the great performance, powerful type system, modern packaging system, etc. But there are definitely still bumps along the way during the learning curve.

In any event, here are various thoughts I jotted down while diving into the project for the first time:

  • The number one thing I wanted/expected to see are some examples. Examples are integrated into cargo (cargo run --example pricing) and I often start there when I'm looking at a project for the first time.

  • On the other hand, there's plenty of tests. (Second step is often git clone then cargo test for me). Perhaps some of the tests include code that would be suitable to use in examples. Do you have any recommendations on which ones to look at?

  • One of the biggest issues I face is needing to use different number types in different situations and performance and ergonomics pitfalls associated with juggling all of them, converting from one to the other, etc. In many cases, correct precision is required and a decimal type is needed. (I have been using the rust wrapper to the decNumber lib for over a year, it seems by my lights to be the best choice at the moment). In many other cases, the performance hit from using decimal is very, very steep compared to the floating point primitives (f64/f32). Rust tends very much towards prudence when it comes to floating point, with a carefully designed api, but it still takes a lot of care to avoid blowups. A third option that sometimes presents itself is to use integer types as fixed-point, for instance when the precision of prices is static. This can be very fast and precise, but it's not always available. I saw that the library generally uses f64, I was wondering if you had further thoughts on how that has worked for you in this application.

  • I'm intrigued by the date functionality here; it might be worth pursuing it as a stand-alone crate. Currently the chrono crate has a pretty good handle on power/completeness/correctness and it works very well with serde for deserialization. However, the main chrono DateTime<Utc> type is somewhat heavyweight: it's an i32 for the date, two u32s for the time, plus an offset enum (which I believe is zero-sized, however). I frequently use nanosecond timestamps stored in an i64/u64 for performance, and there's no good library that I've seen that focuses less on the kitchen sink, and more on the most performant possible way to store/operate on times.

  • Regarding the Error struct defined in core::qm:

    pub struct Error {
        message: String 
    }

    (Haven't been able to tell if this is widely used throughout the library or not). Error-handling has been a moving target in rust, with the std Error trait falling out of favor and several different crates trying to make error-handling more ergonomic have risen and fallen. Personally, I have come to prefer using Error enums defined on a per-crate or per-mod basis. In this case, it might look something like this:

    pub enum Error {
        Io(std::io::Error),
        ShapeError(ndarray::ShapeError)
        // ...
    }

    A lot of the time I keep it even simpler with "zero-size" enums with no data associated with each variant, but the variant name alone is enough to act on the error.

    The advantages of localized, lightweight error types include:

    • In rust, matching on or otherwise manipulating/operating on types is very ergonomic and powerful, while matching/operating on strings is relatively painful, owing to the strict ownership semantics and String/str divergence.

    • If latency is a priority, as it often is in trading, incurring a heap allocation (for the message String) just to handle an error is somewhat wasteful.

    • This is especially true because in idiomatic rust, program crashes are very rare and there's little need to gather as much information as possible for piecing together what went wrong after the fact. Your debugging time is more likely to be spent on the front-end getting the program to compile in the first place.

  • Regarding this comment/question:

    impl Black76 {
        pub fn new() -> Result<Black76, qm::Error> {
    
            // For some reason we cannot use the ? operator for this sort of error.
            // as a workaround, do it manually for now.
            match Normal::new(0.0, 1.0) {
                Ok(normal) => Ok(Black76 { normal: normal }),
                Err(e) => Err(qm::Error::new(&format!("RSStat error: {}", e)))
            }
        }
        // ...
    }

    The Normal::new function from the statrs crate returns Result<T, StatsError>. Since Black76::new returns Result<Self, qm::Error>, you would only be able to use the question mark operator if a From<_> impl exists to convert a StatsError into a qm::Error. You already have a long list of these From<_> impls in core::qm, so perhaps you discovered this along the way.

    Incidentally, the statrs::StatsError is a great example of a robust, but lightweight error enum that covers the various types of errors that can come from the library without any allocations. &'static str (string literals) used in many of the variants are very ergonomic for that purpose.

    In this case, however, handling the error isn't even necessary. The error can only arise if either argument is NaN or a negative value is passed as the std_dev argument, which is clearly is not the case since it's being passed float literals. In my judgement, this is a perfect use of .unwrap() or .expect("statrs::Normal::new(0.0, 1.0)"). I typically note why an unwrap is safe with a quick comment.

  • One test failed on my machine (using nightly-2018-12-02), which seems to be a floating point issue:

    thread 'math::brent::tests::problem_of_the_day' panicked at 'result=192.80753261197134 expected=192.80752497643797', src/math/brent.rs:146:9

Hope these initial comments are helpful to you. I'm looking forward to delving further into this. I'd be interested to hear what the current status on the project is, and if you have any ideas for areas where collaboration would be especially useful.

Jonathan

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions