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

Lazily compute row and column roots when repairing the square #17

Closed
evan-forbes opened this issue Mar 23, 2021 · 2 comments · Fixed by #18
Closed

Lazily compute row and column roots when repairing the square #17

evan-forbes opened this issue Mar 23, 2021 · 2 comments · Fixed by #18
Assignees

Comments

@evan-forbes
Copy link
Member

When repairing the square, we are currently generating roots of each column and row even if there is nil data in that row or col. This is a problem because because if the underlying tree doesn't know how to handle nil values, then it has no choice but to panic. While there are quite a few ways to fix this, I think the fastest way would be to only compute the root of columns and rows that have no nil values. In the context of RepairDatasquare, this means having the ability to generate roots of single columns and rows.

This must be fixed before #235 and #232 can be completed.

@liamsi
Copy link
Member

liamsi commented Mar 23, 2021

While there are quite a few ways to fix this, I think the fastest way would be to only compute the root of columns and rows that have no nil values. In the context of RepairDatasquare, this means having the ability to generate roots of single columns and rows.

That sounds good to me. Do you mind drafting a minimal PR (without tests etc) to see how this would look like in code? At least it sounds like a relatively simple change.

@evan-forbes evan-forbes self-assigned this Mar 23, 2021
@evan-forbes
Copy link
Member Author

I cheated and wrote the code before the issue 😄 #18

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 a pull request may close this issue.

2 participants