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

Typos in README summary table? #34

Closed
mcaceresb opened this issue Jul 27, 2017 · 1 comment
Closed

Typos in README summary table? #34

mcaceresb opened this issue Jul 27, 2017 · 1 comment

Comments

@mcaceresb
Copy link

It seems typos were introduced to the README summary table in commit 00a4e5a.

I could be reading it wrong, but the "Quality problems" column in lines 49 through 69 appears to be shifted one row up for a few hashes: i.e. the problems noted correspond to the hash one row down. In commit 00a4e5a, a hash was added but the quality problems column does not appear to have been copied correctly.

The hashes affected are:

  • City32 vs City64: The former has no issues in the log but is noted to have 2 minor collisions, whereas the latter has no problems noted but has 2 collisions in its log (here).
  • Spooky128 vs xxHash32: The former has no issues in the log but is noted to have collisions with 4-bit differentials (here), whereas the latter has no problems noted but has a collision with 4-bit differentials (here).
  • metrohash128_2 vs metrohash64crc_1 and metrohash64crc_2: The first has no collisions in its log but is noted to have cyclic collisions with 8 byte (which is not even checked in the log, see here), whereas the second metrohash64 is noted to have no problems but also has cyclic collisions with 8 byte here.
mcaceresb added a commit to mcaceresb/stata-gtools that referenced this issue Jul 27, 2017
Enhancements

* `gegen varname = group(varlist)` no longer has holes, as noted in issue
  #4
* `gegen` and `gcollapse` fall back on `collapse` and `egen` in case there
  is a collision. Future releases will implement an internal way to resolve
  collisions. This is not a huge concern, as SpookyHash has no known
  vulnerabilities (I believe the concern raied in issue #2
  was base on a typo; see [here](rurban/smhasher#34))
  and the probability of a collision is very low.
* `gegen varname = group(varlist)` now has a consistency test (though
  the group IDs are not the same as `egen`'s, they should map to the `egen`
  group IDs 1 to 1, which is what the tests now check for).

Bug fixes

* Additional fixes for issue #1
* Apparentlly the argument Stata passes to plugins have a maximum length. The
  code now makes sure chuncks are passed when the PATH length will exceed the
  maximum. The plugin later concatenates the chuncks to set the PATH correctly.
@rurban
Copy link
Owner

rurban commented Jul 27, 2017

Oops, you are right. I shifted it accidently by copy&pasta.
City64 has unexpectedly indeed more collision problems then City32.

rurban added a commit that referenced this issue Jul 27, 2017
Accidently shifted by one by copy&pasta.
Many thanks to Mauricio Caceres Bravo
@rurban rurban closed this as completed Jul 27, 2017
mcaceresb added a commit to mcaceresb/stata-gtools that referenced this issue Jul 27, 2017
gtools-0.6.5 through gtools-0.6.9

Enhancements

* Addressed the possible issue noted in issue
  #3 and the functions now
  use mata and extended macro functions as applicable.
* `gegen varname = group(varlist)` no longer has holes, as noted in issue
  #4
* `gegen` and `gcollapse` fall back on `collapse` and `egen` in case there
  is a collision. Future releases will implement an internal way to resolve
  collisions. This is not a huge concern, as SpookyHash has no known
  vulnerabilities (I believe the concern raied in issue #2
  was base on a typo; see [here](rurban/smhasher#34))
  and the probability of a collision is very low.
* `gegen varname = group(varlist)` now has a consistency test (though
  the group IDs are not the same as `egen`'s, they should map to the `egen`
  group IDs 1 to 1, which is what the tests now check for).
* The function now checks numerical variabes to see if they are integers.
  Working with integers is faster than hashing.
* The function is now smarter about generating targets. In prior versions,
  when the target statistic was a sum the function would force the target
  type to be `double`. Now if the source already exists and is a float, the
  function now checks if the resultimg sum would overflow. It will only
  recast the source as double for collapsing if the sum might overflow, that
  is, if `_N * min < -10^38` or `10^38 < _N * max` (note +/- 10^38 are the
  largest/smallest floats stata can represent; see `help data_types`).

Bug fixes

* `gegen` no longer ignores unavailable options, as noted in issue
  #4, and now it throws an error.
* `gegen varname = tag(varlist)` no longer tags missing values, as noted
  in issue #5
* Additional fixes for issue #1
* Apparentlly the argument Stata passes to plugins have a maximum length. The
  code now makes sure chuncks are passed when the PATH length will exceed the
  maximum. The plugin later concatenates the chuncks to set the PATH correctly.
* Fixed issue #1
* The problem was that the wrapper I wrote to print to the Stata
  console has a maximum buffer size; when it tries to print the
  new PATH it encounters an error when the string is longer than
  the allocated size. Since printing this is unnecessary and
  will only ever be used for debugging, I no longer print the PATH.
* Debugging issue #1
  on github (in particular, `env_set` on Windows).
* Removed old debugging code that had been left uncommented
* Improved out-of-memory message (now links to relevant help section).
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