Kll longs sketch #561
Merged
Kll longs sketch #561
Conversation
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 native longs KLL sketch
In the main code, nearly all the corrections are in the Javadocs.
However, in the test code, there were many places where he was comparing
a long vs a float ('cause he copied the code from the float classes).
Also, he didn't check the test classes with printing enabled. There were
numerous places where the printf format expected an "%f" that should
have been a "%d".
Nonetheless, Zac did an amazing job of creating this KllLongsSketch,
considering the complexity of the code!
I also did a coverage tests and made sure that the coverage of the
"longs" classes were very close to the coverage of the comparable
"doubles" and "floats" classes -- and they were! This means that he was
able to faithfully translate all the relevant tests from float to longs
and I did not find any gaps in testing.
During testing, these changes make sure we testing an expected long (vs a float) to the actual long In a few cases in "doubles sketch" tests I found an expected float being tested against the actual double. These are all "cut and paste" errors.
|
I admit a few of these files are KLL Doubles Sketches where my search string picked up the same errors in those files, so I just corrected them and included them here. Going forward I will create a separate PR for these other extraneous errors. |
|
This is difficult to review because this view does not show the context.
|
AlexanderSaydakov
approved these changes
Jun 3, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
In the main code, nearly all the corrections are in the Javadocs.
However, in the test code, there were many places where he was comparing a long vs a float ('cause he copied the code from the float classes).
Also, he didn't check the test classes with printing enabled. There were
numerous places where the printf format expected an "%f" that should have been a "%d".
Nonetheless, Zac did an amazing job of creating this KllLongsSketch, considering the complexity of the code!
I also did a coverage tests and made sure that the coverage of the "longs" classes were very close to the coverage of the comparable "doubles" and "floats" classes -- and they were! This means that he was able to faithfully translate all the relevant tests from float to longs and I did not find any gaps in testing.