-
Notifications
You must be signed in to change notification settings - Fork 14
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
Test on data folder 2 not passing #7
Comments
Hi @endel, there is one more option. We can add a bool parameter to Delta.Create called useNonStandardOptimizations or something like that and only do this new optimization if that is true. It is up to you, so let me know if you would like that or if you still prefer the revert and I'll make the pull request. |
Although, I'm not really an expert about the algorithm, nor had I performed benchmarks, but I think lines 83 to 87 in, 264ea17#diff-993db0c7e13460f7683a59bed31a7144R83-R87 However, if you're also thinking the same way as mine, or think that the optimization might not be worth it at all, then I would strongly vote that lines 83 to 87 of PR #4, be reverted instead. |
@israellot thoughts? 👀 |
Thanks for the feedback @jasonsparc ! Regarding the unit test, the deltas are compatible as the format hasn't changed, or should be. I believe @igor84 was pointing out that the baked data used to compare against is different from the delta generated after the PR. It might be interesting trying to check why that is and what exactly is different from one output to another. I remember doing these cross-checks though, applying deltas from previous version on new one and vice-versa. Maybe something slipped. |
Just to complement, I believe the deltas generated by the previous version could be different from the deltas generated by the new version, but that doesn't mean they aren't compatible, it only means one has a different set of commands relative to the other, or commands in a different order as the search order is different on both versions. |
@israellot you right, deltas are different but they still work. I tested with and without these changes using files from 60KB to 21MB (with generated deltas ranging from 2KB to 20MB) and difference in speed is somewhere positive, somewhere negative but either way it is negligible (bellow 1%) so I will submit a pull request reverting all the changes. That way deltas will remain the same as in other tools. |
That would need a proper benchmark. |
Sorry, I messed up the pull request. I wanted to submit another one and it got merged into existing pull request. How can I fix that? |
@igor84 You could I believe, by default, force push is disabled only for the |
Thanks @jasonsparc. It seems that fixed it. |
@igor84 btw, I'm excited about that I hope to see it implemented some time soon. 😄 |
Good to hear @jasonsparc :). It is done just for |
fixed #7 test on data folder 2 not passing by reverting performance tweak
It seems the commit 264ea17 "performance tweak" broke the test on input data in folder "2". The delta that is created no longer corresponds to the delta that was generated by the original tool. On the other hand if that new delta is applied to original data, target data is generated correctly.
I can make a pull request with updated delta file in folder 2 if you agree that is ok to do. This will mean generate-deltas.sh script can't be used any more to regenerate delta data.
The text was updated successfully, but these errors were encountered: