Revised Hash functions incorporating changes in the main Stdlib repository. #573

wclodius2 · 2021-11-22T13:41:00Z

Numerous changes were made in the main repository to deal with the lack of double precision in gfortran's implementation for the Mac M1 processor, and a few other issues, meant that the hash functions source code needed to be merged with the main repository. This proved overwhelming so I did a clone of the main repository, copied the changes for the hash function codes into that, made further changes in the code in response to comments on the original PR, and generated a new PR. As with the original

I have created a number of hash functions for incorporation in the Fortran standard library by translating from C or C++ a number of algorithms recommended by Reini Urban who maintains a well respected code, SMHasher, for testing the properties and performance of hash functions. The codes can hash default character strings and integer vectors. The hash functions are:

fibonacci hash: a scalar hash used to map 32 or 64 bit integers to power of two sized hash tables.
32 and 64 bit versions of the FNV-1 and FNV-1a: simple hash functions with excellent performance for small keys, whose poor statistical properties appear to have only a minor impact on the performance of small hash tables.
nmhash32 and nmhash32x: 32 bit hashes with excellent statistical properties, but whose performance, so far, seems to be poor.
water_hash: a 32 bit hash function with excellent statistical performance and very good performance on large keys.
pengy hash: a 64 bit hash function with excellent statistical performance and good performance on large keys.
SpookyV2 hash: a 128 bit hash with excellent statistical performance and excellent performance on large keys.
A number of random seed generators for the above hashes.

Bringing over codes fro original hash_functions branch [ticket: X]

Brought over codes from the original hash_functions branch. [ticket: X]

Brought over code from original hash_functions [ticket: X]

Modified so it doesn't check overflow of integers. [ticket: X]

Added the gcc compiler flag no-range-checks [ticket: X]

Added files with hash function modules to the list of fypp files to be processed. [ticket: X]

Added hash_functions to the test directories [ticket: X]

Added documentation for the hash functions. [ticket: X]

Removed two filesusedd in an older version of hash validation. [ticket: X]

Added generate_key_array.f90 generate_hash_arrays.f90 and hash_validity_test..f90 to implement a new version of the validity test. [ticket: X]

Modified the Makefile so it would compile the new validation. [ticket: X]

Added README>md that is documentation for the code in the hash function validation directory. [ticket: X]

Implemented fixes to various files addressing problems brought up by Jeremie Vandenplas. [ticket: X]

Change a compile flag for gfortran from -fwrapv to -fno-range-check. [ticket: X]

Changed from parentheses to square brackets. [ticket: X]

gareth-nx · 2021-11-24T21:36:05Z

Note the previous PR is here (with initial discussions): #554

gareth-nx

As discussed in the previous pull request, this looks very impressive.

Most of my comments are quite minor.

I appreciate that you've added some regression tests. For now they are outside of the regular stdlib test-suite (due to dependency on C/C++), and I think we should discuss with others whether that is the right approach for now, or whether now is the time to start supporting C/C++ dependencies. I don't have a deep enough knowledge on the software engineering implications to make strong statements about this.

CMakeLists.txt

doc/specs/stdlib_hash_functions.md

src/stdlib_64_bit_hash_functions.fypp

src/stdlib_64_bit_pengy_hashes.fypp

src/stdlib_64_bit_spookyv2_hashes.fypp

src/tests/hash_functions/validation/Makefile.validation

src/tests/hash_functions/validation/README.md

@gareth-nx

Changed three files "CMakeLists.txt", "stdlib_hash_functions.md", and "stdlib_64_bith_hash_functions.fypp"in response to @gareth-nx's comments [ticket: X]

Incorporated more changes in stdlib_hash_functions.md based on comments by @garreth-nx. [ticket: X]

jvdp1

Thank you @wclodius2 . Here are some comments on the specs.

In general it is an impressive amount of work, with well detailed documentation.

I will now focus on the code

doc/specs/index.md

doc/specs/stdlib_hash_functions.md

Renamed stdlib_hash_functions.md to stdlib_hash_procedures.md. [ticket: X]

Renamed stdlib_hash_functions.md to stdlib_hash_proceedures.md effectively deleting it. [ticket: X]

Co-authored-by: Jeremie Vandenplas <[email protected]>

Changed documentation in ways suggested by Jeremie's comments. [ticket: X]

…hash_functions2

jvdp1

Here are some additional comments

src/stdlib_32_bit_hash_functions.fypp

jvdp1 · 2021-11-28T17:46:41Z

src/stdlib_32_bit_nmhashes.fypp

+!! The algorithms come in multiple versions, depending on whether the
+!! vectorized instructions SSE2 or AVX2 are available. As neither instruction
+!! is available in portable Fortran 2008, the algorithms that do not use these
+!! instructions are used.


Maybe for further improvements: would it be possible that the compiler select the right code when e.g., mentioning the option for supporting AVX2?

Sure if a preprocessor can inquire of its existence from the compiler.

An alternative to preprocessing, could be to use a logical parameter and rely upon the compiler's ability to remove dead code (similar to the little-endian parameter). A second option is to pack the different implementations in submodules, which are then substituted at link time based upon a flag to the build system. But personally I'm happy with the portable version.

src/stdlib_32_bit_nmhashes.fypp

jvdp1 · 2021-11-28T18:49:03Z

src/stdlib_64_bit_hash_functions.fypp

+    interface spooky_init
+
+         module subroutine spookysubhash_init( self, seed )
+            type(spooky_subhash), intent(out) :: self
+            integer(int_hash), intent(in)     :: seed(2)
+        end subroutine spookysubhash_init
+
+    end interface spooky_init
+
+
+    interface spooky_update
+
+         module subroutine spookyhash_update( spooky, key )
+            type(spooky_subhash), intent(out) :: spooky
+            integer(int8), intent(in)         :: key(0:)
+        end subroutine spookyhash_update
+
+    end interface spooky_update
+
+
+    interface spooky_final
+
+         module subroutine spookyhash_final(spooky, hash_code)
+            type(spooky_subhash), intent(inout) :: spooky
+            integer(int_hash), intent(inout)    :: hash_code(2)
+        end subroutine spookyhash_final
+
+    end interface spooky_final


If I am correct, there are private interfaces. If so, could they be moved to the submodules? Or what is the reasons to keep them here?

They are the basis of an incremental hasher, and were originally public.I eventually decided that including the incremental hasher complicated the API in ways that were inappropriate for an initial release, though they could be easily be made public.

src/stdlib_64_bit_spookyv2_hashes.fypp

wclodius2 · 2021-11-28T21:30:12Z

I am having some problems with git. I accepted a couple of of suggestions of @jvdp1 to modify the stdlib_hash_functions.md in https://github.com/wclodius2/stdlib/tree/hash_functions2, but most of his suggested changes were significantly modified, and entered on my local copy, including the implicit suggestion that stdlib_hash_functions.md be renamed stdlib_hash_procedures.md. I therefore wanted to pull https://github.com/wclodius2/stdlib/tree/hash_functions2, merge it with my local copy, and then push the resulting code back to the GitHub branch. I initially tried to initially push my code to the GitHub branch

git push william
To github.com:wclodius2/stdlib.git
 ! [rejected]        hash_functions2 -> hash_functions2 (fetch first)
error: failed to push some refs to 'github.com:wclodius2/stdlib.git'
hint: Updates were rejected because the remote contains work that you do
hint: not have locally. This is usually caused by another repository pushing
hint: to the same ref. You may want to first integrate the remote changes
hint: (e.g., 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

I didn't find the 'Note about fast-forwards' useful so I tried pulling the remote, and ended up pulling https://github.com/fortran-lang/stdlib

git pull origin master
hint: Pulling without specifying how to reconcile divergent branches is
hint: discouraged. You can squelch this message by running one of the following
hint: commands sometime before your next pull:
hint: 
hint:   git config pull.rebase false  # merge (the default strategy)
hint:   git config pull.rebase true   # rebase
hint:   git config pull.ff only       # fast-forward only
hint: 
hint: You can replace "git config" with "git config --global" to set a default
hint: preference for all repositories. You can also pass --rebase, --no-rebase,
hint: or --ff-only on the command line to override the configured default per
hint: invocation.
remote: Enumerating objects: 34, done.
remote: Counting objects: 100% (34/34), done.
remote: Compressing objects: 100% (19/19), done.
remote: Total 34 (delta 16), reused 23 (delta 14), pack-reused 0
Unpacking objects: 100% (34/34), 10.37 KiB | 442.00 KiB/s, done.
From https://github.com/fortran-lang/stdlib
 * branch            master     -> FETCH_HEAD
   57cfaf0..3af3259  master     -> origin/master
Auto-merging src/Makefile.manual
Auto-merging src/CMakeLists.txt
Auto-merging doc/specs/index.md
Auto-merging Makefile.manual
Auto-merging CMakeLists.txt
Merge made by the 'recursive' strategy.
 .github/PULL_REQUEST_TEMPLATE.md |  4 +++
 .github/workflows/CI.yml         |  4 ++-
 API-doc-FORD-file.md             |  3 ++
 CMakeLists.txt                   | 12 ++++++--
 Makefile.manual                  |  6 ++++
 VERSION                          |  1 +
 ci/fpm-deployment.sh             |  6 ++++
 ci/fpm.toml                      |  2 +-
 doc/specs/index.md               |  1 +
 doc/specs/stdlib_version.md      | 64 +++++++++++++++++++++++++++++++++++++++++++
 src/CMakeLists.txt               |  4 +++
 src/Makefile.manual              |  3 +-
 src/common.fypp                  |  3 ++
 src/stdlib_version.fypp          | 65 ++++++++++++++++++++++++++++++++++++++++++++
 14 files changed, 173 insertions(+), 5 deletions(-)
 create mode 100644 .github/PULL_REQUEST_TEMPLATE.md
 create mode 100644 VERSION
 create mode 100644 doc/specs/stdlib_version.md
 create mode 100644 src/stdlib_version.fypp

So now my local copy is up to date WRT https://github.com/fortran-lang/stdlib, but not WRT https://github.com/wclodius2/stdlib/tree/hash_functions2. Another git push failed with its earlier error message. I suspect I have to change my remotes, but how I am to change them I don't know.

git remote -v
origin	https://github.com/fortran-lang/stdlib (fetch)
origin	https://github.com/fortran-lang/stdlib (push)
william	[email protected]:wclodius2/stdlib.git (fetch)
william	[email protected]:wclodius2/stdlib.git (push)

Any suggestions?

jvdp1 · 2021-11-28T22:03:32Z

After commiting suggestions on github, you must pull the remote branch into your local one as follows:

git pull william hash_functions2

If there are no conflicts, then all changes will be merged in your local branch, and you can then push your branch.
If there are commites, you need first to resolve them manually in your local project.

awvwgk · 2021-12-19T09:23:05Z

How critical is it that I change the names in the documentation to lower case?

More for consistency with existing specs, so far we were using always lowercase for procedure names. I don't really mind, but this is something that was noticeable on the first glance.

jvdp1 · 2021-12-19T09:27:59Z

How critical is it that I change the names in the documentation to lower case?

More for consistency with existing specs, so far we were using always lowercase for procedure names. I don't really mind, but this is something that was noticeable on the first glance.

I opened a PR against the @wclodius2 branch for modifying all names. hopefully I missed none of them...

doc/specs/stdlib_hash_procedures.md

Co-authored-by: Jeremie Vandenplas <[email protected]>

Update the description of testing procedures

Changed capitalized words to lowercase + some typos

…sh_functions2

…hash_functions2

wclodius2 · 2021-12-20T08:10:20Z

I have now merged in

@jvdp1 lower causing of the UPPER case procedure names, and typo fixes;
The recent changes in the master standard library

The result passes all checks, and I believe is ready to merge into the stdlib master.

src/CMakeLists.txt

Myhash16

awvwgk

Thanks a lot, it looks great. I'm happy to get it merged.

A final suggestion, we should add the new modules to the CHANGELOG.md.

Updated CHANGELOG.md to reflect the new modules stdlib_hash_32bit and stdlib_hash_64bit. [ticket: X]

wclodius2 · 2021-12-22T08:05:30Z

I believe I have properly added the modules to CHANGELOG.md.

gareth-nx · 2021-12-22T09:24:01Z

I think this is ready to be merged? Any outstanding issues @milancurcic @ivan-pi ?

jvdp1

Just added the names of the public procedures in the CHANGELOG.md

CHANGELOG.md

ivan-pi

I have no major comments anymore. Thanks for all of your work, especially William.

I think I noticed some small formatting issues in the itemized sections of the markdown document, but would need to render it first. These can be dealt with in a separate PR.

jvdp1 · 2021-12-22T17:26:50Z

@milancurcic I believe all your comments were answered. However, I am afraid I could miss some. Could you check please, and merge this PR if it it the case?

milancurcic

Thank you William and reviewers for this great addition!

wclodius2 added 15 commits November 21, 2021 14:44

Added hash function performace comparison and validation codes.

f1490bc

Bringing over codes fro original hash_functions branch [ticket: X]

Added hash function codes

8cd2f2e

Brought over codes from the original hash_functions branch. [ticket: X]

Add files to test hash function performance

2b0a9ec

Brought over code from original hash_functions [ticket: X]

Modified to have no-range-check

b004215

Modified so it doesn't check overflow of integers. [ticket: X]

Modified to have no-range-checks

98e22dd

Added the gcc compiler flag no-range-checks [ticket: X]

Added hash modules to the compile list

d778671

Added files with hash function modules to the list of fypp files to be processed. [ticket: X]

Added hash_functions to the test directories

461543a

Added hash_functions to the test directories [ticket: X]

Added stdlib_hash_functions.md

29a5297

Added documentation for the hash functions. [ticket: X]

Removed two F90 files

165f4ff

Removed two filesusedd in an older version of hash validation. [ticket: X]

Added three source code files

5c22bb2

Added generate_key_array.f90 generate_hash_arrays.f90 and hash_validity_test..f90 to implement a new version of the validity test. [ticket: X]

Modified Makefile.validation

1cf7bcd

Modified the Makefile so it would compile the new validation. [ticket: X]

Added documentations

cf76cc1

Added README>md that is documentation for the code in the hash function validation directory. [ticket: X]

Fixes to various files

f35875b

Implemented fixes to various files addressing problems brought up by Jeremie Vandenplas. [ticket: X]

Change s compile flag

1661cff

Change a compile flag for gfortran from -fwrapv to -fno-range-check. [ticket: X]

Changed brackets on TOC

542741d

Changed from parentheses to square brackets. [ticket: X]

jvdp1 mentioned this pull request Nov 24, 2021

Hash functions #554

Closed

wclodius2 changed the title ~~Revised Hash functions incorporating changes in the min Stdlib repository.~~ Revised Hash functions incorporating changes in the main Stdlib repository. Nov 25, 2021

gareth-nx reviewed Nov 26, 2021

View reviewed changes

wclodius2 added 2 commits November 26, 2021 13:38

Changes in response to Gareth's comments

1769bb7

Changed three files "CMakeLists.txt", "stdlib_hash_functions.md", and "stdlib_64_bith_hash_functions.fypp"in response to @gareth-nx's comments [ticket: X]

More changes from Gareth

e62abdf

Incorporated more changes in stdlib_hash_functions.md based on comments by @garreth-nx. [ticket: X]

jvdp1 reviewed Nov 27, 2021

View reviewed changes

wclodius2 and others added 5 commits November 27, 2021 16:39

Renamed stdlib_hash_functions.md

96ed2e7

Renamed stdlib_hash_functions.md to stdlib_hash_procedures.md. [ticket: X]

Deleted file

87869ad

Renamed stdlib_hash_functions.md to stdlib_hash_proceedures.md effectively deleting it. [ticket: X]

Update doc/specs/stdlib_hash_functions.md

d449bd0

Co-authored-by: Jeremie Vandenplas <[email protected]>

Changes suggested by Jermie Vandenplas

874d1c4

Changed documentation in ways suggested by Jeremie's comments. [ticket: X]

Merge branch 'master' of https://github.com/fortran-lang/stdlib into …

1588f93

…hash_functions2

jvdp1 reviewed Nov 28, 2021

View reviewed changes

jvdp1 reviewed Dec 19, 2021

View reviewed changes

doc/specs/stdlib_hash_procedures.md Outdated Show resolved Hide resolved

wclodius2 and others added 8 commits December 19, 2021 08:09

Update doc/specs/stdlib_hash_procedures.md

8ce29b2

Co-authored-by: Jeremie Vandenplas <[email protected]>

Merge pull request #10 from jvdp1/myhash15

5863d78

Update the description of testing procedures

Update doc/specs/stdlib_hash_procedures.md

3d29b92

Update doc/specs/stdlib_hash_procedures.md

ebcf239

Merge remote-tracking branch 'wclodius2/hash_functions2' into myhash14

d7217a6

Merge pull request #9 from jvdp1/myhash14

95fcc8f

Changed capitalized words to lowercase + some typos

Merge branch 'hash_functions2' of github.com:wclodius2/stdlib into ha…

ae18abc

…sh_functions2

Merge branch 'master' of https://github.com/fortran-lang/stdlib into …

a7944b1

…hash_functions2

awvwgk reviewed Dec 20, 2021

View reviewed changes

src/CMakeLists.txt Outdated Show resolved Hide resolved

jvdp1 and others added 5 commits December 21, 2021 11:09

rename hash submodule files

a144d69

update CMakeLists.txt

bcc0cc2

update Makefile.manual

cf3b60b

change specs

1a1b005

Merge pull request #11 from jvdp1/myhash16

ae194d5

Myhash16

awvwgk approved these changes Dec 21, 2021

View reviewed changes

awvwgk removed the reviewers needed This patch requires extra eyes label Dec 21, 2021

Updated CHANGELOG.md

c82ec64

Updated CHANGELOG.md to reflect the new modules stdlib_hash_32bit and stdlib_hash_64bit. [ticket: X]

jvdp1 approved these changes Dec 22, 2021

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

CHANGELOG.md Show resolved Hide resolved

jvdp1 added 2 commits December 22, 2021 10:33

Added public procedures in CHANGELOG.md

28f2d6f

Merge branch 'master' into hash_functions2

f0629ae

ivan-pi approved these changes Dec 22, 2021

View reviewed changes

milancurcic approved these changes Dec 22, 2021

View reviewed changes

milancurcic merged commit 9f1aa24 into fortran-lang:master Dec 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revised Hash functions incorporating changes in the main Stdlib repository. #573

Revised Hash functions incorporating changes in the main Stdlib repository. #573

wclodius2 commented Nov 22, 2021

gareth-nx commented Nov 24, 2021

gareth-nx left a comment

jvdp1 left a comment

jvdp1 left a comment

jvdp1 Nov 28, 2021

wclodius2 Nov 29, 2021

ivan-pi Dec 16, 2021

jvdp1 Nov 28, 2021

wclodius2 Nov 29, 2021

wclodius2 commented Nov 28, 2021

jvdp1 commented Nov 28, 2021

awvwgk commented Dec 19, 2021

jvdp1 commented Dec 19, 2021

wclodius2 commented Dec 20, 2021

awvwgk left a comment

wclodius2 commented Dec 22, 2021

gareth-nx commented Dec 22, 2021

jvdp1 left a comment

ivan-pi left a comment

jvdp1 commented Dec 22, 2021

milancurcic left a comment

Revised Hash functions incorporating changes in the main Stdlib repository. #573

Revised Hash functions incorporating changes in the main Stdlib repository. #573

Conversation

wclodius2 commented Nov 22, 2021

gareth-nx commented Nov 24, 2021

gareth-nx left a comment

Choose a reason for hiding this comment

jvdp1 left a comment

Choose a reason for hiding this comment

jvdp1 left a comment

Choose a reason for hiding this comment

jvdp1 Nov 28, 2021

Choose a reason for hiding this comment

wclodius2 Nov 29, 2021

Choose a reason for hiding this comment

ivan-pi Dec 16, 2021

Choose a reason for hiding this comment

jvdp1 Nov 28, 2021

Choose a reason for hiding this comment

wclodius2 Nov 29, 2021

Choose a reason for hiding this comment

wclodius2 commented Nov 28, 2021

jvdp1 commented Nov 28, 2021

awvwgk commented Dec 19, 2021

jvdp1 commented Dec 19, 2021

wclodius2 commented Dec 20, 2021

awvwgk left a comment

Choose a reason for hiding this comment

wclodius2 commented Dec 22, 2021

gareth-nx commented Dec 22, 2021

jvdp1 left a comment

Choose a reason for hiding this comment

ivan-pi left a comment

Choose a reason for hiding this comment

jvdp1 commented Dec 22, 2021

milancurcic left a comment

Choose a reason for hiding this comment