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

Undefined self attribute in LiquidLegions #128

Open
riemanli opened this issue Oct 15, 2021 · 1 comment
Open

Undefined self attribute in LiquidLegions #128

riemanli opened this issue Oct 15, 2021 · 1 comment

Comments

@riemanli
Copy link
Contributor

The function frequency_sample accesses the undefined attribute self.mask. In addition, I notice that the unit tests doesn't cover the usage of function frequency_sample .

@c-dickens
Copy link

I have also just noticed this issue. I think an incorrect copy has been made from cascading_legions.py where this attribute is defined. Notice that in that file, a comment is made that self.mask is used for the key aggregation.

A similar comment is made in liquid_legions.py referencing the same_key_aggregator function. For this reason, I think the dictionary self. unique stores the keys. By this I mean that self.unique operates in accordance with the key array from the LiquidLegions paper. Then replacing self.mask with self.unique should work as a drop in replacement.

However, I also noticed that there is no unit-testing coverage and I don't think the use of frequency capping (f_max from the paper) has been implemented in the current frequency estimation function, although it should be quite straightforward to perform this after the full histogram is returned.

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

No branches or pull requests

2 participants