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

Fix import of inner count min for TopK import #35

Merged
merged 1 commit into from
Jul 7, 2021

Conversation

JustroX
Copy link
Contributor

@JustroX JustroX commented Jul 3, 2021

Hello, thanks for creating this library! It's been very helpful.

I'm currently working with the TopK data structure and encountered a bug.
It seems that it's throwing this error whenever I am trying to update an imported TopK.

TypeError: this._sketch.update is not a function

This is the code for the bug replication
https://stackblitz.com/edit/node-kcv3z7?file=index.js

// Initialize topK
const topk = new TopK(k, error_rate, accuracy);
topk.add('Alice');
topk.add('Bob');

// Save topk
const json = topk.saveAsJSON();
const str = JSON.stringify(json);

// Import topK
const new_topk = TopK.fromJSON(JSON.parse(str));

// Update topk
new_topk.add('Carl');  // this throws

I figured that this might be due to the inner count-min sketch not being properly initialized during import.

I am not that confident that I understand what the decorators do, but based on how the inner heap is imported I made the following fix in the TopK class.

@Field<CountMinSketch>(
  (sketch: CountMinSketch) => sketch.saveAsJSON(),
  (json: any) => CountMinSketch.fromJSON(json)
)
private _sketch: CountMinSketch

I also made the corresponding tests for this. I hope this fixes the bug.

@Callidon Callidon merged commit 93fa979 into Callidon:master Jul 7, 2021
@JustroX JustroX deleted the fix-exported-topk-update branch July 13, 2021 07:06
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