Conversation

jvdp1

Addition of sort_adj to sort an rank 1 array in the same order as an input array.
It is basically an extension of the original sort_index procedure, by making the array index intent(inout), instead of intent(out) only.

To be discussed:

  • Name of the procedure: currently called sort_adj, but I think another (more appropriate) name should be found
  • Name of the arguments in the API, especially of the associated array currently called index: suggestions of a more appropriate name?
  • Is the current implementation of sort_index still valid/appropriate/useful?

@jalvesz sort_adj could replace your internal sorting procedure in your sparse implementation. If not, I think it is still a nice addition to stdlib.

@jvdp1jvdp1 requested review from perazz and a team July 9, 2024 19:52
@jalvesz

What about naming the secondary array array_adjoint or adjoint_array to be explicit about its role?

@perazz

Thank you for this PR @jvdp1. I agree with @jalvesz and like the adjoint_array name.

Regarding your questions:

  • I believe the original sort_index is appropriate because it represents an argsort, that is a widely used operation. So I think that the former API would better be maintained (just call sort_adj with [(j,j=1,n)] input for the list). No idea why the former name was chosen, but I think now it would be late to change it.
  • In my library, this function is named sort_and_list, I agree it's not a great name. A probably better option could be sort_pair, as in other languages you can do it with a custom comparison function.

These arguments:

${t1}$, intent(inout) :: array(0:)
${ti}$, intent(inout) :: index(0:)

could become

         ${t1}$, intent(inout)         :: sorted_array(0:) 
         ${ti}$, intent(inout)         :: adjoint_list(0:) 

to signal that sorting is performed based on the first argument.

!! https://.com/rust-lang/rust/blob/90eb44a5897c39e3dff9c7e48e3973671dcd9496/src/liballoc/slice.rs#L2159
!! but modified to return an array of indices that would provide a stable
!! sort of the rank one `ARRAY` input.
!! ([Specification](../page/specs/stdlib_sorting.html#sort_adj-creates-an-array-of-sorting-indices-for-an-input-array-while-also-sorting-the-array))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be updated

@jvdp1

Thank you @jalvesz and @perazz for the suggestions.
Based on them I called the new procedure sort_adjoint and the associated array adjoint_array. I kept the name array for the sorted array, as it is used for the other procedures.

sort_index just calls sort_adjoint with an initialised index array.

integer, allocatable :: array(:)
real, allocatable :: adjoint(:)

array = [5, 4, 3, 1, 10, 4, 9]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great example @jvdp1. Could it be made into a test as well? The test would check that array(i)>=array(i-1), and that nint(adjoint(i),kind=${ik}$)==array(i)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Pending to add one test program, I think this is ready to merge.

@jalvesz

@perazz @jvdp1 for the test program, maybe something like one of the tests I wrote for the sparse matrices could work?:

    subroutine test_coo2ordered(error)
        !> Error handling
        type(error_type), allocatable, intent(out) :: error
        type(COO_sp_type) :: COO
        integer :: row(12), col(12)
        real    :: data(12)

        row = [1,1,1,2,2,3,2,2,2,3,3,4]
        col = [2,3,4,3,4,4,3,4,5,4,5,5]
        data = 1.0

        call from_ijv(COO,row,col,data)

        call coo2ordered(COO,sort_data=.true.)
        call check(error, COO%nnz < 12 .and. COO%nnz == 9 )
        if (allocated(error)) return

        call check(error, all(COO%data==[1,1,1,2,2,1,2,1,1])  )
        if (allocated(error)) return

        call check(error, all(COO%index(1,:)==[1,1,1,2,2,2,3,3,4])  )
        if (allocated(error)) return

        call check(error, all(COO%index(2,:)==[2,3,4,3,4,5,4,5,5])  )
        if (allocated(error)) return

    end subroutine 

Applying the sorting to row and having col follow. For the sparse matrices I also remove duplicates after sorting, here I think that step is not required.

Sign up for free to join this conversation on . Already have an account? Sign in to comment
None yet
None yet

Successfully merging this pull request may close these issues.