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

Simplify error handling a bit #32

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vihu
Copy link
Contributor

@vihu vihu commented Oct 14, 2024

Summary

Rather than crashing because of an expect statement in the code, I have added thiserror library and simplified a bit of error handling so the callers can handle it a bit more gracefully.

NOTE: There's more occurrences of expect statement being used throughout. I have only replaced one which I found happening more often in my project. A more thorough handling of errors is probably wiser in the long term.

Summary
----
Rather than crashing because of an `expect` statement in the code, I
have added `thiserror` library and simplified a bit of error handling so
the callers can handle it a bit more gracefully.

NOTE: There's more occurrences of `expect` statement being used
throughout. I have only replaced one which I found happening more often
in my project. A more thorough handling of errors is probably wiser in
the long term.
@tom-whitehead
Copy link
Owner

Hey @vihu, Unfortunately this is a breaking change requiring a major version bump, so I would prefer to avoid adding a new error type as I think people would be matching on it. Unfortunately, I didn't know about #[non_exhaustive] when I wrote this! I am building up a list of things I want to change when I release a new major version.

The reason for all of the expects is partially because I suck at Rust 😅, but also partially because I thought they were all theoretically unreachable. In the most part, I'm expecting a node id to be in the tree of nodes from which I took it in the first place. This should be true and if it's not it's more likely that it's a bug. Do you have a toy dataset or a test case I can reproduce it with? (sorry will also try and look at your other PR again later)

@tom-whitehead
Copy link
Owner

@vihu I just saw the test from your other PR hits this panic. When there's only one cluster and it's the single root cluster I need to skip the epsilon check. I will separately raise a PR for that. I think this should only be hit for very small datasets when allow_single_cluster is true. It looks like fixing this will fix the test in your other PR too.
Thanks for unearthing all these bugs.

@tom-whitehead
Copy link
Owner

@vihu this is the PR if you want to try it out.

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.

2 participants