Update to include int32 hashmap keytype #778

chuckyvt · 2024-03-27T02:26:13Z

This is a fairly minor update to include int32 key types to hash maps. I have tested this a good bit in my own code. It has worked well and is a nice addition to the hashmap capabilities.

Update to stdlib_hashmap_wrappers.f90 that includes support for int32 vector key types.

jvdp1

Thank you @chuckyvt. This is indeed a useful procedure to avoid multiple ...transfer(value, 1_int8).... with the default integer on the user side.
Could you also udpate the specs, please?
Could you also add a test for this specific procedure, please?
Should it be a corresponding get function (quite a long time I did use hashmap, so this question might be irrelevant)?

src/stdlib_hashmap_wrappers.f90

Co-authored-by: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>

Added get_inte32_key function.

chuckyvt · 2024-03-30T03:09:32Z

In progress code updates. Added a 'get_int32_key' function to match set function.

Fixed end subroutine bug

gareth-nx

Thanks for this @chuckyvt. I agree that some tests/specs are needed and understand this is in progress. But it looks like a nice addition.

src/stdlib_hashmap_wrappers.f90

Minor comment cleanup

Update to include native support for int32 key types.

chuckyvt · 2024-04-04T01:35:26Z

Requested code updates are complete. Also updated the specs document. The testing item is a bit more work however. I could use some help or suggestions on the best approach. All the current tests are based around native int8 keys. For example, there are no test cases I can find for the existing character strings? My thought was it might make sense to add an extract test file dedicated to non int8 keys. Character key, int32 key, and could be extended in the future.

gareth-nx · 2024-04-06T07:21:15Z

Thanks @chuckyvt. The addition of extra tests sounds good. Indeed, we will have to add tests for other reasons too (not necessarily in this pull request), see #785.

Updated test_maps.fypp to include tests for int32 and character key types, in addition to base int8.

chuckyvt · 2024-04-15T02:04:37Z

Have pushed an updated to test_maps.fypp that includes tests for int32 and character key types, in addition to the existing base int8 keys. I'm not very familiar with the test drive framework, so I tried to stay as close as possible to the existing testing layout.

jvdp1

Thank you @chuckyvt for adding the tests. I am a bit puzzled by your approach (but it does not mean it is wrong). Could you add a few comments to help the readers and future developers, please? MY main question is to understand why test_8_bits was changed to an array (see comments below).
Note: would it be an idea to add an new array test_32_bits and duplicate the tests of test_8_bits for 32bits (and leave unchanged test_8_bits)?

test/hashmaps/test_maps.fypp

jvdp1 · 2024-04-15T05:16:47Z

test/hashmaps/test_maps.fypp

@@ -53,10 +54,10 @@ contains
            type(error_type), allocatable, intent(out) :: error

            type(chaining_hashmap_type)   :: map
-            integer(int8)             :: test_8_bits(test_size)
+            integer(int8)             :: test_8_bits(test_size,key_types)

            call generate_vector(test_8_bits)


test_8_bits is not a vector anymore.
Could you explain why test_8_bits is changed to an array? I am not sure to understand the aim here.

The approach is to create a unique int8 vector for each key type that can then be used via transfer or other mechanism to generate a unique key hash for the test program. Using a single int8 vector will result in duplicate keys and mapping conflicts, so that must be managed. There are other approaches, but this seemed to be the easiest to add and is extensible to future key types. For example, we could add a dedicated int32 vector generation function, but not sure how that improves on using transfer with an int8 array to create an int32 array, and would require a dedicated function for each key type. (Versus for example if there is want to add int64 or real key types, increase the size of test_8_bits as needed and add appropriate transfer function).

Its also worth noting that the structure of test_maps.fypp is not ideal with the code base being repeated for open and chaining maps. I believe refactoring this code into a single source approach should be done before any additional function additions.

Hopefully this is straightforward with fypp.

Its also worth noting that the structure of test_maps.fypp is not ideal with the code base being repeated for open and chaining maps. I believe refactoring this code into a single source approach should be done before any additional function additions.

Could you open an issue for this, please?

jvdp1 · 2024-04-15T05:29:34Z

test/hashmaps/test_maps.fypp

+            call set( key, test_8_bits( index2:index2+test_block-1, 1 ) )
+            call map % key_test( key, present )
+            call check(error, present, "Int8 KEY not found in map KEY_TEST.")
+
+            call set( key, transfer( test_8_bits( index2:index2+test_block-1, 2 ), [0_int32] ) )
+            call map % key_test( key, present )
+            call check(error, present, "Int32 KEY not found in map KEY_TEST.")
+
+            call set( key, to_string( transfer( test_8_bits( index2:index2+test_block-1, 3 ), 0_int32 ) ) )


Could you add a comment to explain the different tests (and their purposes)? It might help reviewers and future developers.

I've added some additional comments per discussion above. Feel free to add more. I've also removed the character key type test. The code worked fine on my computer, but failed the CI checks for reasons I don't understand. It was a nice to have, but out of scope for this pull request which is for int32 key types.

Also, per the note above, I think test_maps.fypp should be refactored to remove duplicate code prior to adding other key type checks to make code addition less error prone.

I've added some additional comments per discussion above.

Great. Thank you.

I've also removed the character key type test. The code worked fine on my computer, but failed the CI checks for reasons I don't understand. It was a nice to have, but out of scope for this pull request which is for int32 key types.

I agree with you. Let's keep this PR focused on int32

test/hashmaps/test_maps.fypp

Co-authored-by: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>

Added addition commenting. Removed the character key type test as that was failing CI checks for unknown reasons.

…into hashmap_int32_key

gareth-nx

This looks good. I have one query though about transfer when the mold argument is not allocated -- is this standard? I wonder if this could be related to the issue with strings as keys that you mentioned earlier.

gareth-nx · 2024-04-16T07:09:54Z

src/stdlib_hashmap_wrappers.f90

+        type(key_type), intent(in)              :: key
+        integer(int32), allocatable, intent(out) :: value(:)
+
+        value = transfer( key % value, value )


Here I wonder whether it matters that value is unallocated in the RHS call to transfer (since it will be deallocated on input if allocated)?

I've asked this question at fortran discourse here.

Thanks for asking. Intuitively it seems like it shouldn't matter since its only pulling type info, not size. But I also did a quick search and didn't come up with anything. We could hardcode it to [0_int8], but I don't like that quite as much.

src/stdlib_hashmap_wrappers.f90

test/hashmaps/test_maps.fypp

gareth-nx · 2024-04-16T07:13:34Z

test/hashmaps/test_maps.fypp

@@ -53,10 +54,10 @@ contains
            type(error_type), allocatable, intent(out) :: error

            type(chaining_hashmap_type)   :: map
-            integer(int8)             :: test_8_bits(test_size)
+            integer(int8)             :: test_8_bits(test_size,key_types)

            call generate_vector(test_8_bits)


Hopefully this is straightforward with fypp.

test/hashmaps/test_maps.fypp

Added doc strings to explain the key_type approach in test_map.fypp. Indentation cleanup Added pure specifier to get and set int32 key type routines.

chuckyvt · 2024-04-18T11:57:57Z

Note with the latest submit I added pure to the int32 get and set subroutines. Eventually I think adding this to the other interfaces makes sense but is probably a separate pull request. (Also can make sure there are no issues with pure with the int32 type)

typo fix

jvdp1

LGTM. Pending the issue with transfer, it can be merged IMO. Thank you @chuckyvt for this PR.

gareth-nx · 2024-04-19T15:23:20Z

I'm also happy for this to be merged. It looks like the use of transfer with an unallocated mold is OK.

Do we have a record of what usage was causing segfaults? Might come in handy.

jvdp1 · 2024-04-25T16:46:36Z

I'll merge this PR. Thank you @chuckyvt

Update to include int32 hashmap keytype

65bdc89

Update to stdlib_hashmap_wrappers.f90 that includes support for int32 vector key types.

jvdp1 reviewed Mar 27, 2024

View reviewed changes

src/stdlib_hashmap_wrappers.f90 Outdated Show resolved Hide resolved

src/stdlib_hashmap_wrappers.f90 Outdated Show resolved Hide resolved

chuckyvt and others added 2 commits March 27, 2024 08:56

Update src/stdlib_hashmap_wrappers.f90

418cb82

Co-authored-by: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>

Update stdlib_hashmap_wrappers.f90

561cee2

Added get_inte32_key function.

Bug fix

f10f1b7

Fixed end subroutine bug

gareth-nx reviewed Apr 3, 2024

View reviewed changes

src/stdlib_hashmap_wrappers.f90 Outdated Show resolved Hide resolved

src/stdlib_hashmap_wrappers.f90 Outdated Show resolved Hide resolved

chuckyvt added 2 commits April 3, 2024 19:59

Minor comment cleanup

7cf7352

Minor comment cleanup

Update stdlib_hashmaps.md

35ed826

Update to include native support for int32 key types.

chuckyvt added 3 commits April 13, 2024 08:11

Merge branch 'master' into hashmap_int32_key

1724a4f

Add character and int32 keytype tests

cb53f85

Update test_maps to include key types

fe3fffc

Updated test_maps.fypp to include tests for int32 and character key types, in addition to base int8.

jvdp1 reviewed Apr 15, 2024

View reviewed changes

chuckyvt and others added 3 commits April 15, 2024 21:08

Update test/hashmaps/test_maps.fypp

fab4a21

Co-authored-by: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>

Update test_maps.fypp

9b80107

Added addition commenting. Removed the character key type test as that was failing CI checks for unknown reasons.

Merge branch 'hashmap_int32_key' of https://github.com/chuckyvt/stdlib …

9ebb0c2

…into hashmap_int32_key

gareth-nx reviewed Apr 16, 2024

View reviewed changes

stdlib_hashmap_wrappers and test_maps.fypp

822b5c9

Added doc strings to explain the key_type approach in test_map.fypp. Indentation cleanup Added pure specifier to get and set int32 key type routines.

Update test_maps.fypp

f35386f

typo fix

jvdp1 approved these changes Apr 19, 2024

View reviewed changes

gareth-nx approved these changes Apr 19, 2024

View reviewed changes

jvdp1 merged commit f44133b into fortran-lang:master Apr 25, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to include int32 hashmap keytype #778

Update to include int32 hashmap keytype #778

chuckyvt commented Mar 27, 2024

jvdp1 left a comment

chuckyvt commented Mar 30, 2024

gareth-nx left a comment •

edited

Loading

chuckyvt commented Apr 4, 2024

gareth-nx commented Apr 6, 2024

chuckyvt commented Apr 15, 2024

jvdp1 left a comment •

edited

Loading

jvdp1 Apr 15, 2024

chuckyvt Apr 16, 2024 •

edited

Loading

chuckyvt Apr 16, 2024 •

edited

Loading

gareth-nx Apr 16, 2024

jvdp1 Apr 19, 2024

jvdp1 Apr 15, 2024

chuckyvt Apr 16, 2024

jvdp1 Apr 19, 2024

gareth-nx left a comment

gareth-nx Apr 16, 2024

gareth-nx Apr 18, 2024

chuckyvt Apr 18, 2024

gareth-nx Apr 16, 2024

chuckyvt commented Apr 18, 2024

jvdp1 left a comment

gareth-nx commented Apr 19, 2024

jvdp1 commented Apr 25, 2024

Update to include int32 hashmap keytype #778

Update to include int32 hashmap keytype #778

Conversation

chuckyvt commented Mar 27, 2024

jvdp1 left a comment

Choose a reason for hiding this comment

chuckyvt commented Mar 30, 2024

gareth-nx left a comment • edited Loading

Choose a reason for hiding this comment

chuckyvt commented Apr 4, 2024

gareth-nx commented Apr 6, 2024

chuckyvt commented Apr 15, 2024

jvdp1 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chuckyvt Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

chuckyvt Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gareth-nx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chuckyvt commented Apr 18, 2024

jvdp1 left a comment

Choose a reason for hiding this comment

gareth-nx commented Apr 19, 2024

jvdp1 commented Apr 25, 2024

gareth-nx left a comment •

edited

Loading

jvdp1 left a comment •

edited

Loading

chuckyvt Apr 16, 2024 •

edited

Loading

chuckyvt Apr 16, 2024 •

edited

Loading