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

Rename hnsw.init_k to lantern_hnsw.init_k #219

Merged
merged 3 commits into from
Nov 3, 2023

Conversation

var77
Copy link
Collaborator

@var77 var77 commented Oct 31, 2023

Rename hnsw.init_k parameter to lantern_hnsw.init_k to match the naming convention we have for lantern_hnsw.ef_search parameter

@var77 var77 requested a review from Ngalstyan4 October 31, 2023 13:42
Copy link

github-actions bot commented Oct 31, 2023

Benchmarks

metric old new pct change
recall (after create) 0.740 0.740 -
recall (after insert) 0.762 0.746 -2.10%
select bulk tps 488.520 472.036 -3.37%
select bulk latency (ms) 15.345 16.009 +4.33%
select bulk latency (stddev ms) 3.092 2.973 -3.85%
create latency (ms) 1209.939 1216.015 +0.50%
insert bulk tps 11.154 11.020 -1.20%
insert bulk latency (ms) 89.651 90.740 +1.21%
insert bulk latency (stddev ms) 3.534 2.824 -20.09%
disk usage (bytes) 6348800.000 6348800.000 -

Copy link

codecov bot commented Oct 31, 2023

Codecov Report

Merging #219 (2b926ea) into main (8110b58) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #219   +/-   ##
=======================================
  Coverage   77.92%   77.92%           
=======================================
  Files          20       20           
  Lines        1468     1468           
  Branches      370      370           
=======================================
  Hits         1144     1144           
  Misses        160      160           
  Partials      164      164           
Files Coverage Δ
src/hnsw/options.c 87.93% <100.00%> (ø)

@Ngalstyan4
Copy link
Contributor

should this only be done when pgvector is also installed?
we only use lantern_hnsw if hnsw is already taken by pgvector

@var77
Copy link
Collaborator Author

var77 commented Oct 31, 2023

I did this change to match the naming with the other variable we have lantern_hnsw.ef_search . As that also has the prefix lantern_, we can decide to use hnsw or lantern_hnsw (or use lantern_hnsw only when pgvector is installed) what do you think?

@Ngalstyan4
Copy link
Contributor

hm, lantern_hnsw was supposed to only be for pgvector compatibility.
hnsw must exist otherwise. (lantern_hnsw can exist otherwise as well, but hnsw MUST exist)
So, this should be changed accordingly

@Ngalstyan4 Ngalstyan4 merged commit 69a9e3d into main Nov 3, 2023
@Ngalstyan4 Ngalstyan4 deleted the varik/lantern-hnsw-init-k branch November 3, 2023 07:14
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