dcjones / coitrees

A very fast interval tree data structure
MIT License
111 stars 8 forks source link

Iteration results differ between BasicCOITree and NeonCOITree #16

Open vsbuffalo opened 11 months ago

vsbuffalo commented 11 months ago

I was using this library in a project and noticed a subtle potential bug: taking a Vec<Interval<()>> and creating both BasicCOITree and NeonCOITree objects, and then iterating over their intervals with .iter() produces different results. I have created a MRE (mre.rs)

use coitrees::{BasicCOITree, Interval, IntervalTree, NeonCOITree};

fn main() {
    let mut intervals: Vec<Interval<()>> = Vec::new();

    intervals.push(Interval { first: 0, last: 10, metadata: () } );
    intervals.push(Interval { first: 11, last: 14, metadata: () } );

    let basic_trees: BasicCOITree<(), usize> = BasicCOITree::new(&intervals);

    for interval in basic_trees.iter() {
        println!("basic tree interval: {:?}", interval);
    }

    let neon_trees: NeonCOITree<(), usize> = NeonCOITree::new(&intervals);

    for interval in neon_trees.iter() {
        println!("neon tree interval: {:?}", interval);
    }
}

which can be placed in the examples/ directory and run with cargo run --example mre to see the behavior. The output I see is:

basic tree interval: Interval { first: 0, last: 10, metadata: () }
basic tree interval: Interval { first: 11, last: 14, metadata: () }
neon tree interval: Interval { first: -1, last: 11, metadata: () }
neon tree interval: Interval { first: 10, last: 15, metadata: () }

I believe the potential issue could be due to these lines (https://github.com/dcjones/coitrees/blob/main/src/neon.rs#L62-L81). Unfortunately, I do not know enough about targeting these specific architectures to know what is the right way to address this issue and create a pull request. Perhaps these lines are correct, but the iterator for NeonCOITree needs to be adapted to output offset intervals.

cauliyang commented 9 months ago

@vsbuffalo, Great find! It appears we overlooked adding an offset when converting the coitree to an iterator. This issue is present in both aux and neon. I've taken care of it and fixed the bug with commit d4921e04f8efcb09e061ef74bc170cd10e671c86. You can find this fix in one of my ongoing pull requests at https://github.com/dcjones/coitrees/pull/12.

vsbuffalo commented 9 months ago

Awesome, thanks @cauliyang!