Sorting2

[wclodius2]

Merged inconsistent changes in doc/specs/stdlib_sorting.mb and src/stdlib_sorting.fypp.

[gareth-nx]

Looks great -- noting I already reviewed almost the same code at in the related pull request. I hope we get some more reviewers and this can be merged.

[jvdp1]

Thank you for these implementations. Here are some general comments. Based on your answers , I will continue the review.

[w6ws]

A few API suggestions:

1.) In subroutine sort_index - the index, work, and iwork arrays should probably be intent(out), not intent(inout). Same with the work array in ord_sort.

2.) All three subroutines should have an optional return status that indicates success or failure. Right now there are some hard stops in the code - specifically when allocate is called. It would be good to have the ability to avoid these.

3.) Shouldn't all three have a 'reverse' argument so the caller can specify either ascending or decending sort order?

FWIW, in some code I've worked on, we use private types with public parameters for such things. E.g.,:

module sorting_mod
implicit none
private
...
type :: sortFlag_t ! private type
logical :: descending
end type

type(sortFlag_t), parameter, public :: &
SORTFLAG_ASCENDING = sortFlag_t (.false.), &
SORTFLAG_DESCENDING = sortFlag_t (.true.)

While in the case of sort_index there are only two options, so not strictly needed, the technique is general to other areas of a library where many more options for an argument might be offered. It ensures that the compiler can enforce correct option selections at compile time. And it provides clear indication of what is being specified - rather than some "magic number". It also removes some need for run-time validation checks.

[wclodius2]

I am on travel and it is awkward for me to find the time to do the changes, but I will see what I can do.

[wclodius2]

I have committed and pushed changes suggested by Jeremie.

[wclodius2]

@w6ws sorry for my late response, but I have been busy on my trip.

Seems like a pain to require the manual steps of reversing inputs and outputs to get a descending order. Especially since, perhaps stability aside, it is only a matter of which comparison operator to use within the sorting algorithms. Again, user-friendliness.

While logically the only difference is in the comparison operators in practice it wouldn't be implemented that way as it would result in a large duplication of code. It would instead be implemented using a reverse procedure.

[jvdp1]

1. The errors can only be "caught" for a memory allocation problem. With lazy allocation on Linux. and virtual memory on all systems, it is not clear what can cause an allocation error, or that such an error is recoverable. I am right now inclined to remove the stat argument to the allocate statements and simply let a memory error halt processing and let the processor report the problem if it occurs. This will allow my procedures to be pure.

In general I like library routines to return control to their caller when they detect errors - then let the caller decide what to do. For example in the case of sort_ord and sort_index, besides allocation errors, checks could also be made to ensure the scratch and output arrays are large enough. Just makes things more user friendly. But good point on enabling the procedures to be pure.

The topic of handling errors come often in stdlib. Would it be an idea to add an intent(out) integer (e.g., called info, as in BLAS/LAPACK), that would return a value different than 0, with allocation erros, and on checks on scratch and output arrays? With such an approach, the stat argument in the allocate statement could be kept, while error stop could be removed, and the procedure be pure.

I had the impression that @w6ws wanted the argument to be optional, in which case I would want the ERROR STOP to be present and we would not gain purity.

I agree with you: if the argument is optional, then an error stop is likely to be needed. However, I would not make such an argument optional (similarly to the info argument in many procedures of LAPACK, or other error arguments in other libraries): up to the user to use/check it or not.

Did you have any opinion on the @w6ws proposal for a reverse optional argument? I am concerned at the impact on the complexity of the API, and also on its impaction my time. I want to get this PR done, though the error handling is the bigger potential project.

I would suggest to keep this implementation of reverse for another issue/PR. I also like the approach proposed by @w6ws in this comment. This would allow different options in the future, like sorting in increasing or decreasing order, and in a stable or unstable approach,... However, this requires more discussions on the API and the approach before implementation, because it could influence future procedures.

@awvwgk @milancurcic any opinions on this PR?

[w6ws]

First, thank you for accepting my suggestion on intent(out) stuff!

Second, on the reverse capability - I'd think that if some of the sorts only support sorting in one direction, you'll have a lot of users scratching their heads wondering why. (Like me.)

Years ago I added a sorting routine to the ESMF library - https://github.com/esmf-org/esmf/blob/develop/src/Infrastructure/Util/src/ESMF_UtilSort.cppF90. This factored out a bunch of little sorting routines that had been sprinkled throughout the library. It is based on the LAPACK SLASRT sort. I templatized it for various data types using the C preprocessor (same technique is also used in many other parts of the ESMF library.) Taking a look at it, the IF ELSE ENDIF blocks for direction are done at an outermost level. The loops and interiors of the blocks are identical - except for comparison direction. A characteristic of the SLASRT-based code is that it doesn't use much temporary storage space. So passing in, or creating local, work arrays are not needed.

I've also, for personal use, implemented simplified versions of the HPF SORT_UP, SORT_DOWN, GRADE_UP, and GRADE_DOWN functions. In these cases, the direction of the sort is embedded in the entry point name. So no conditional code is needed for sort order. FWIW, I implemented SORT_UP and SORT_DOWN again using SLASRT-based code. Basically copied the input array to the functions result area - then did the sort on the result area. I based GRADE_UP and GRADE_DOWN on the Numerical Recipies INDEXX routine. INDEXX also uses minimal scratch space. So again, large scratch arrays weren't needed.

Third, on error processing - yes I would suggest making an error return argument optional. Referring back to the ESMF library again, the coding style we used was to make an optional Return Code (rc=) argument as the last argument of every library routine. We also strongly encouraged users to check the RC. If you look at ESMF_UtilSort, it checks for a couple of things and if it finds an error it will (optionally) set the RC to something other than ESMF_SUCCESS, and then return. So it returns bad results and (optionally) an error RC, rather than a hard stop. The ESMF philosophy, for better or worse, has been to encourage programmers to always check the RC for success. I should note that ESMF normally operates in a MPI environment. Some users routinely run climate and weather models with hundreds or thousands of MPI processes. So if one thread abends, it can cause a calamity. We really wanted the users to handle their own errors.

[jvdp1]

While logically the only difference is in the comparison operators in practice it wouldn't be implemented that way as it would result in a large duplication of code. It would instead be implemented using a reverse procedure.

I though a bit about this issue. If the main implementation is to replace all comparison operators (e.g., >) by its opposite (e.g., <), then I think it could be easily done by adding an additional fypp loop. In this case, no code should be duplicated. @wclodius2 I can give a try if it is a possible solution for you.

[wclodius2]

While logically the only difference is in the comparison operators in practice it wouldn't be implemented that way as it would result in a large duplication of code. It would instead be implemented using a reverse procedure.

I though a bit about this issue. If the main implementation is to replace all comparison operators (e.g., >) by its opposite (e.g., <), then I think it could be easily done by adding an additional fypp loop. In this case, no code should be duplicated. @wclodius2 I can give a try if it is a possible solution for you.

At least for stdlib_sorting_ord_sort more than one form of comparison operator is used, i.e., > and >=. Other than that you are the expert on FYPPand if you think it is straightforward go for it.

[jvdp1]

While logically the only difference is in the comparison operators in practice it wouldn't be implemented that way as it would result in a large duplication of code. It would instead be implemented using a reverse procedure.

I though a bit about this issue. If the main implementation is to replace all comparison operators (e.g., >) by its opposite (e.g., <), then I think it could be easily done by adding an additional fypp loop. In this case, no code should be duplicated. @wclodius2 I can give a try if it is a possible solution for you.

At least for stdlib_sorting_ord_sort more than one form of comparison operator is used, i.e., > and >=. Other than that you are the expert on FYPPand if you think it is straightforward go for it.

@wclodius2 I opened a PR against your branch sorting2.
I added:

• an additional option in sort and ord_sort for reverse sorting
• check (error stop) to be sure that it is failing when tests are not succesful

Note: I didn't update the specs, in case the proposed approach is not correct/appropriate.

Also, it could be cleaner to create a new branch (e.g., "sorting3") from "sorting2", to keep the lattter a bit clean.

[jvdp1]

@wclodius2 I opened this PR with some updates in the specs regarding the reverse optional arg.

[jvdp1]

Some additional comments on some checks. If you agree, I can have a look to implement them.

[jvdp1]

With all the changes made, it is ready to be merged IMO

Thank you @wclodius2 for this module

[jvdp1]

Could someone with a write access review this too? @milancurcic? @awvwgk? @ivan-pi?

[awvwgk]

This is not a full review, I followed the discussion in this thread a bit and started looking over the specs, but won't be able to give an in-depth review in the next days. Since we already have two approving reviews here, I'll approve based on @gareth-nx's review to make it count to towards the merge requirements. Sorry for being a bit missing in action here.

[wclodius2]

This branch now has two approvals but still needs someone to merge the PR.