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

add similarity test #2

Merged
merged 9 commits into from
May 11, 2024
Merged

add similarity test #2

merged 9 commits into from
May 11, 2024

Conversation

jianshu93
Copy link
Contributor

Add similarity test, about 10% error for Jaccard at about 0.01, consistent with the paper. Please merge if you find it useful. I would also suggest a comparison with SetSketch, please see the issue I opened.

Thanks,

Jianshu

Similarity test, about 10% error for Jaccard at about 0.01, consistent with the paper.
@lukaslueg
Copy link
Owner

Thanks for the PR. CI seems legit, please address those

remove unused declare
@jianshu93
Copy link
Contributor Author

Should be working this time?

Jianshu

update error calculation
update formula
@lukaslueg
Copy link
Owner

Nope :-)

If you run cargo fmt // cargo clippy locally, you should get the same result as CI. Otherwise, refer to the log

@jianshu93
Copy link
Contributor Author

jianshu93 commented May 9, 2024

Hello @lukaslueg ,

It passed this time on my end.

Thanks,

Jianshu

src/lib.rs Outdated
let jexact = inter as f64 / vbmax as f64;
let sketch1: Sketch = va;
let sketch2: Sketch = vb;
let _union_size = sketch1.clone().union(&sketch2).cardinality();
Copy link
Owner

Choose a reason for hiding this comment

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

If _union_size is unused, the whole line should be removed.

src/lib.rs Outdated
}

Copy link
Owner

Choose a reason for hiding this comment

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

This blank line is causing a rustfmt error

@jianshu93
Copy link
Contributor Author

Fixed!

@lukaslueg
Copy link
Owner

The final blank line is still causing a rustfmt error

@jianshu93
Copy link
Contributor Author

Sorry for the lack of experience in using clippy and format. I love it after trying so many times (no such tools that can help with formatting to have very high quality code in other languages). I normally just did cargo build and then pass. By the way for the nightly rust v1.80, there are errors there for clippy. It will take some time to have the problem on stable rust, but just to let you know.

Jianshu

@lukaslueg lukaslueg merged commit dbf80f1 into lukaslueg:master May 11, 2024
5 checks passed
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.

None yet

2 participants