Skip to content

Index thinning#226

Open
fedorgrab wants to merge 3 commits into
mainfrom
fg/issue-225
Open

Index thinning#226
fedorgrab wants to merge 3 commits into
mainfrom
fg/issue-225

Conversation

@fedorgrab

Copy link
Copy Markdown
Collaborator

No description provided.

@fedorgrab fedorgrab self-assigned this Jul 16, 2026
@sonarqubecloud

Copy link
Copy Markdown

@sjfleming sjfleming left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Some comments about the "carryover" concept and whether it can be eliminated. If it's necessary, that's okay. Just looking for a potential simplification

need = training_sample_size - total_training_rows
train_dfs.append(df.iloc[:need])
carry_df = df.iloc[need:]
total_training_rows += need

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

is need right, or is it len(df) - need ?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

oh wait, it looks like it is need



@_tiledb_retry
def _write_carryover(index: vs.IVFFlatIndex, ids: np.ndarray, emb: np.ndarray, normalize: bool) -> int:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The "carryover" concept introduced in this PR is a little complex. I gather it's the "extra" cells opened in a file but not used to fill the initial index. And we want to use them later as updates.

I wonder: is there a way to eliminate the need for the carryover concept entirely? Something like:

  • just allow the size of the initially-created index to be a bit flexible, so no carryover is needed

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

or:

  • have a first phase where you load embedding files, keep only allowed cells in memory, and then write them out to a new file every time you get enough cells. in the new files, n_cells is controlled and you know all cells are allowed. then do what you used to do

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think I like this second idea, it will make all the subsequent phases to be identical no matter if we need thinning or not. Good one, thanks!

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