Support sorting arrays of bitsets #723

degawa · 2023-06-26T16:14:42Z

To support sorting arrays of bitsets, type(bitset_64) and type(bitset_large) are added as arguments of procedures in the stdlib_sorting.

The tasks done are summarized as follows:

Added bitset types to ALT_NAME in the stdlib_sorting modules.
Added some tests.
Updated and fixed API-doc and spec related to the stdlib_sorting module.
- The overview of sorting was not updated to respect the original intent.
  
  The Fortran Standard Library therefore provides a module, stdlib_sorting, with procedures to sort arrays of simple intrinsic numeric types
- Performance benchmarks were not updated because the same computer environment was not available.
Added an example.
Changed the argument's intent from out to inout in the procedure for overloading the assignment operator for bitset types.
- This change avoids assignment failure when an assignment such as array(1)=array(1) is performed.
- Executed examples that were using the bitset types and confirmed those finished successfully.

closes #720

- BITSET_KINDS_TYPES is not needed to support sorting bitsets.

- tests for `ord_sort` procedure - tests for `sort` procedure - tests for `sort_index` procedure - verification procedures for ascending and reverse sorting

to reduce the time for testing

to support assignment of the same variable, i.e., `set=set`

jvdp1

Thank you. On overall LGTM. Here are a few comments.

example/sorting/example_sort_bitsetl.f90

jvdp1 · 2023-06-28T20:04:50Z

src/stdlib_bitsets.fypp

@@ -1170,8 +1170,8 @@ module stdlib_bitsets
 !! Version: experimental
 !!
 !! Used to define assignment for `bitset_large`.
-            type(bitset_large), intent(out) :: set1
-            type(bitset_large), intent(in)  :: set2
+            type(bitset_large), intent(inout) :: set1


Why is this needed?
And if it is needed, I guess the corresponding interfaces should be also modified for bitset_64.
It might be wise to open another PR with these changes only related to stdlib_bitset

Should the corresponding specs be modified too?

This change and the change you mentioned below have been done to fix unexpected behavior when assigning the bitset_large type.

Without this fix, the tests for sorting bitset_large in the following environment will fail:

msys2-build (MSYS, x86_64) in ci_windows.yml

msys2-build (MINGW32, i686) in ci_windows.yml

msys2-build (MINGW32, i686) in ci_windows.yml

Build (macos-latest, 11, cmake) in CI.yml

Build (macos-latest, 12, cmake) in CI.yml

As you said, opening a new issue for further discussion is better.

I share the reason for those modification.

The component blocks of set1 with the intent(out) attribute is not allocated at the beginning of the assignment procedure. Therefore, when size( set2 % blocks, kind=bits_kind ) is executed, an error occurs that blocks have not been allocated. To avoid this error, I modified the intent attribute from intent(out) to intent(inout).

Although I could not find a direct description of this situation in the Fortran Standard, the following statements are relevant:

The INTENT attribute for an allocatable dummy argument applies to both the allocation status and the
definition status. An actual argument that corresponds to an INTENT (OUT) allocatable dummy argument
is deallocated on procedure invocation (9.7.3.2).

When a variable of derived type is deallocated, any allocated allocatable subobject is deallocated. If an error condition occurs during deallocation, it is processor dependent whether an allocated allocatable subobject is deallocated.

The INTENT (OUT) attribute for a nonpointer dummy argument specifies that the dummy argument becomes undefined on invocation of the procedure, except for any subcomponents that are default-initialized (7.5.4.6).

An error still occurred in the assignment procedure after the modification.
allocate( set1 % blocks( size( set2 % blocks, kind=bits_kind ) ) ) gives the error "set1%blocks is already allocated" when the same variable of the bitset_large type was specified on both sides of the assignment operator. This sometimes happens when the array indices for the bitset array to be sorted are the same.
Since set1 and set2 are the same when the same array element is now specified on both sides of the assignment operator, it is natural that this error occurs.
Therefore, the automatic allocation functionality is used instead of explicit allocation.

Similar modifications are not required for type(bitset_64). This is because type(bitset_64) uses an integer corresponding integer(int64) for storing bitset and thus uses a built-in assignment operation.

Ok. Thank you for the explanation. A new PR with a test replicating this issue would be useful.
Otherwise, this PR looks good to me.

I followed your comment and opened another issue #726 with replication instructions because discussing it in more detail would be better.

src/stdlib_bitsets_large.fypp

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

degawa · 2023-07-19T00:38:18Z

I removed the user-defined assignment operator for the bitset_large type. Then I included the test @jvdp1 added in the pull request #727 and confirmed that all the tests passed.
These commits solve the issue of the self-assignment for the bitset_large type.

jvdp1 · 2023-07-19T12:48:46Z

I removed the user-defined assignment operator for the bitset_large type. Then I included the test @jvdp1 added in the pull request #727 and confirmed that all the tests passed. These commits solve the issue of the self-assignment for the bitset_large type.

Nice that it solves the issue.
To keep "one idea, one PR", this change should be merged in #727 first.

degawa · 2023-07-20T15:22:34Z

Nice that it solves the issue.
To keep "one idea, one PR", this change should be merged in #727 first.

I understand the review policy, but I have already merged the changes made in #727. How should I handle those?
Should I ask to remove the assign_large procedure in #727 for solving the self-assignment problem?
Many thanks for your advice.

jvdp1 · 2023-07-22T17:27:50Z

I understand the review policy, but I have already merged the changes made in #727. How should I handle those? Should I ask to remove the assign_large procedure in #727 for solving the self-assignment problem? Many thanks for your advice.

Sorry for the confusion. #727 should be merged first, with the remove assign_large procedure (I just included it in the PR. Could you review it, please?). Then you can rebase your branch against the master branch.

* add explicity in test_stdlib_bitset_large * add test following issue #726 * remove assign bitset_large * Update src/stdlib_bitsets_large.fypp * Update src/stdlib_bitsets.fypp * remove assign bitset_64 * update specs

src/stdlib_bitsets.fypp

src/stdlib_bitsets_large.fypp

jvdp1

thank you @degawa the PR #727 has been merged. I will now merge your PR, which became much simpler and with a single aim.

degawa added 18 commits June 25, 2023 17:03

added BITSET_KINDS and BITSET_TYPES

07c0e10

- BITSET_KINDS_TYPES is not needed to support sorting bitsets.

added use statement for stdlib_bitsets

04104b1

added BITSET_TYPES_ALT_NAME

b482534

appended ALT_NAME of bitsets to IRSC ALT_NAME

799ce0d

added whitespace after comma

bd92e37

added tests for bitset_large and bitset_64

4d3057a

- tests for `ord_sort` procedure - tests for `sort` procedure - tests for `sort_index` procedure - verification procedures for ascending and reverse sorting

aligned the frame of the table of results

a99f19d

fixed incorrect right-hand side value

6154e2b

added an example for sorting array of bitset_large

423a8b4

updated api-docs

0ee5fb1

fixed enclosure symbol mismatches

a9254e1

added a newline to enable the unordered list

e1b520c

replaced tab with spaces

185c695

aligned indent width

1b0d50a

fixed missing stdlib_kinds module

47e9c19

reduced the size of bitset arrays

e27e9af

to reduce the time for testing

changed intent from out to inout

e7e6e41

to support assignment of the same variable, i.e., `set=set`

changed component assignment operation

5f1195f

jvdp1 reviewed Jun 28, 2023

View reviewed changes

degawa and others added 6 commits July 2, 2023 14:26

specified the entity to be used in the example

4100e61

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

specified the entity to be used in the example

bdd6dcc

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

deleted tailing "l"

61029ee

removed block structures

7d6d979

add explicity in test_stdlib_bitset_large

de2dbbb

add test following issue fortran-lang#726

96763e1

jvdp1 mentioned this pull request Jul 8, 2023

PR related to #726 and #723 #727

Merged

degawa added 2 commits July 15, 2023 17:14

Merge branch 'pullreq_727' into support-sorting-bitsets

1905c80

removed assign_large

efbc0cc

Merge branch 'master' into support-sorting-bitsets

916d83d

jvdp1 reviewed Aug 8, 2023

View reviewed changes

src/stdlib_bitsets.fypp Outdated Show resolved Hide resolved

jvdp1 reviewed Aug 8, 2023

View reviewed changes

src/stdlib_bitsets_large.fypp Outdated Show resolved Hide resolved

Removed additional empty lines introduced by resolving conflicts

e661cd2

jvdp1 approved these changes Aug 8, 2023

View reviewed changes

jvdp1 merged commit bdec191 into fortran-lang:master Aug 8, 2023
15 checks passed

degawa deleted the support-sorting-bitsets branch August 18, 2023 00:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support sorting arrays of bitsets #723

Support sorting arrays of bitsets #723

degawa commented Jun 26, 2023 •

edited

Loading

jvdp1 left a comment

jvdp1 Jun 28, 2023

jvdp1 Jun 28, 2023

degawa Jul 2, 2023

jvdp1 Jul 2, 2023

degawa Jul 8, 2023

degawa commented Jul 19, 2023

jvdp1 commented Jul 19, 2023

degawa commented Jul 20, 2023

jvdp1 commented Jul 22, 2023

jvdp1 left a comment

Support sorting arrays of bitsets #723

Support sorting arrays of bitsets #723

Conversation

degawa commented Jun 26, 2023 • edited Loading

jvdp1 left a comment

Choose a reason for hiding this comment

jvdp1 Jun 28, 2023

Choose a reason for hiding this comment

jvdp1 Jun 28, 2023

Choose a reason for hiding this comment

degawa Jul 2, 2023

Choose a reason for hiding this comment

jvdp1 Jul 2, 2023

Choose a reason for hiding this comment

degawa Jul 8, 2023

Choose a reason for hiding this comment

degawa commented Jul 19, 2023

jvdp1 commented Jul 19, 2023

degawa commented Jul 20, 2023

jvdp1 commented Jul 22, 2023

jvdp1 left a comment

Choose a reason for hiding this comment

degawa commented Jun 26, 2023 •

edited

Loading