You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Fix NVQParameters.idStringFor format string - #780
NVQParameters.idStringFor passed two arguments to a three-placeholder format string ("NVQ_%s_%d_%s"), so it threw MissingFormatArgumentException. The trailing _%s is removed, so the cache key is now NVQ_<dataset>_<nSubVectors>, matching the PQ key's style.
Nothing constructs NVQParameters today, so behavior doesn't change; this removes a latent failure for when NVQ is wired up as a compressor option.
Testing: adds CompressorParametersTest, which checks the NVQ cache key.
Cached NVQ compressors would be reloaded as PQ (fixed in #776)
The key fix here is right, but it makes NVQ caching reachable, and the load path can't read what it saves. With idStringFor working, supportsCaching() == true takes effect: Grid.getCompressor saves the NVQuantization to pq_cache/NVQ_<ds>_<n>. On the next run, Grid.loadFromCache reads every cache file with ProductQuantization.load. NVQ's layout starts with a version int, which PQ reads as a v0 centroid length, so you'd get an exception or a corrupt ProductQuantization where an NVQuantization was expected. The test here only checks the key string, never a save and reload, so it doesn't catch this.
loadFromCache is code that #776 (mine) moves and rewrites, so I've fixed it there rather than ask for it here:
Adds CompressorParameters.loadCompressor(RandomAccessReader). The default throws UnsupportedOperationException, like idStringFor. PQParameters uses ProductQuantization.load and NVQParameters uses NVQuantization.load.
Grid.loadFromCache now calls cp.loadCompressor(...) instead of always ProductQuantization.load.
Includes the same idStringFor change as this PR ("NVQ_%s_%d"), because NVQ caching can't be exercised without it.
Adds TestCompressorCaching, which round-trips PQ and NVQ through loadCompressor and checks the NVQ key.
How the two interact:
Either order merges cleanly. The idStringFor line is identical in both, and your test is a new file under jvector-examples/src/test, separate from the new one in Refactor grid #776.
No broken window in between. Nothing constructs NVQParameters outside tests yet, and the YAML compression types don't include NVQ. So if this merges first, the PQ-load path can't be reached before Refactor grid #776 lands.
Your test's supportsCaching() assertion is accurate once Refactor grid #776 is in.
So this can merge as is, or you can close it in favour of #776. Either works for me.
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #779.
NVQParameters.idStringForpassed two arguments to a three-placeholder format string ("NVQ_%s_%d_%s"), so it threwMissingFormatArgumentException. The trailing_%sis removed, so the cache key is nowNVQ_<dataset>_<nSubVectors>, matching the PQ key's style.Nothing constructs
NVQParameterstoday, so behavior doesn't change; this removes a latent failure for when NVQ is wired up as a compressor option.Testing: adds
CompressorParametersTest, which checks the NVQ cache key.