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

Rust panic #398

Open
acpeakhour opened this issue Aug 30, 2023 · 8 comments
Open

Rust panic #398

acpeakhour opened this issue Aug 30, 2023 · 8 comments

Comments

@acpeakhour
Copy link

Hi,

I get the following panic:

thread panicked at 'index out of bounds: the len is 32 but the index is 32', deps/rcf/Rust/src/samplerplustree/cut.rs:72:24

Seems the while loop doesn't break and dim is still incremented. I noticed that the Java code has additional code to handle this case.

Is it just appropriate in the Rust version to check if dim == points.len() and decrement? Or would implementing the additional logic
found in the Java version be required?

@acpeakhour acpeakhour changed the title Rust panick Rust panic Aug 30, 2023
@sudiptoguha
Copy link
Contributor

This is likely the same issue as #374

Would you be able to drop in the cut.rs from the current PR (just changing that one file should not affect anything else)? Or is this arising in the current PR itself? The current PR performs the check against the length.

@sudiptoguha
Copy link
Contributor

And one of the main instigations for the new PR was to not have panics but at least enable the capability of propagating errors (but there are still ways to go in terms of messages and error details -- hopefully they can be handled iteratively)

@acpeakhour
Copy link
Author

Amusingly, I just converted the logic from the Java version. I'll try your update.

The RCF v4 update is interesting, I'm trying to understand everything you are saying. We currently struggle with 10gb heap running many multi dimensional trcf's. The way I read that is that we can have a single forest handling many?

@sudiptoguha
Copy link
Contributor

sudiptoguha commented Aug 31, 2023

"we can have a single forest handling many" -> yes, with a price. If the time series are very different then precision/recall (measured via implanted anomalies) will reduce because the model can get confounded. This is ameliorated slightly based on the late detection (relative_index field) -- but again, mileage may vary.

@acpeakhour
Copy link
Author

I get a new error now, could there be some dependent code elsewhere?

deleting wrong node; looking for 4511 found 5311
want 1.1760913 found 2.0043213
want 0 found 0
want 0 found 0
want 0 found 0
want 0.90309 found 0.69897
want 0 found 0
want 0 found 0
want 0 found 0
want 1.2787536 found 1.1760913
want 0 found 0
want 0 found 0
want 0 found 0
want 1.0791812 found 0.7781513
want 0 found 0
want 0 found 0
want 0 found 0
want 1.7160033 found 1.8129133
want 0 found 0
want 0 found 0
want 0 found 0
want 0.47712123 found 0.60206
want 0 found 0
want 0 found 0
want 0 found 0
want 1.6627579 found 1.6232493
want 0 found 0
want 0 found 0
want 0 found 0
want 0.845098 found 0.845098
want 0 found 0
want 0 found 0
want 0 found 0
thread panicked at 'explicit panic', deps/rcf/Rust/src/samplerplustree/randomcuttree.rs:222:17

@sudiptoguha
Copy link
Contributor

What is happening is that the code is looking for (want) a specific value to delete and finding something else in that position
that explains "deleting wrong node; looking for 4511 found 5311. This would happen if the trees are corrupted (that is, violate the assumption that every point on left is less or equal in that splitting coordinate, and everything on right is greater). If the cut subroutine produces a cut that does not separate the bounding box into two parts (when called from the add subroutine), then the tree could get corrupted. Are you seeing this error using the restored check-pointed/serialized forests or from new ones?

@acpeakhour
Copy link
Author

I upgraded to your rcfv4 branch. I'm pleased to report:

  • no more crashes
  • uses 1/3 of the heap
  • runs significantly faster

well done.

@sudiptoguha
Copy link
Contributor

Thanks and thanks for trying the new branch. I am happy that it worked for you.

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

No branches or pull requests

2 participants