Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Just brings the code for the new sorting. Intentionally have not yet … #1541

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

atrayano
Copy link
Contributor

@atrayano atrayano commented Jun 3, 2022

…changed the names to avoild name collision, nor included in the build system

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Trivial change (affects only documentation or cleanup)

Checklist:

  • I have tested this change with a run of GEOSgcm (if non-trivial)
  • [x ] I have added one of the required labels (0 diff, 0 diff trivial, 0 diff structural, non 0-diff)
  • I have updated the CHANGELOG.md accordingly following the style of Keep a Changelog

…changed the names to avoild name collision, nor included in the build system
@atrayano atrayano added 0 Diff The changes in this pull request have verified to be zero-diff with the target branch. 🚫 Contingent - DNA Do Not Approve (DNA). These changes are contingent on other PRs labels Jun 3, 2022
@atrayano atrayano requested a review from tclune June 3, 2022 16:38
@atrayano atrayano requested a review from a team as a code owner June 3, 2022 16:38
contains

subroutine SORTL(A,Bi,Bl,Br,Bd,Ci,Cl,Cr,Cd,Di,Dl,Dr,Dd,DIM)
integer(kind=8), intent(INOUT) :: A(:)
Copy link
Member

Choose a reason for hiding this comment

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

I imagine we'll need to generalize these for NAG, since kind=8 doesn't work there...

Comment on lines +111 to +118
if (present(Bi)) then
call QSORTL44(A,Bi,Dm,len,1,0)
elseif(present(Bl))then
call QSORTL84(A,Bl,Dm,len,1,0)
elseif(present(Br))then
call QSORTL44(A,Br,Dm,len,1,0)
elseif(present(Bd)) then
call QSORTL84(A,Bd,Dm,len,1,0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah - no, this is a terrible/unacceptable style. In theory all 4 of these args could be present. The proper way todo this is to have separate interfaces for each case.

Comment on lines +144 to +146
// If the length of the sort exceeds a critical value (MAX_SORT_LEN),
// it halves it repeatedly until it is below this value, does
// order LEN/MAX_SORT_LEN sorts, and shuffles these in ascending order.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment leaves out the "why". Is it to give better performance? To avoid some memory limitation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only Max could answer "why". My guess is for better performance... I do not think we exceed the threshold currently. Which brings the question, if the "swap-and-shuffle" scheme was well tested

Copy link
Contributor

@weiyuan-jiang weiyuan-jiang Jun 3, 2022

Choose a reason for hiding this comment

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

I remember one thing. I changed the sort so it pickd the middle item as pivot point. Before that, it chose the first one as the pivot point. So if an array is already sorted, it takes O( n^2) to sort again. If the array is very large, it would be very slow. It helps if it limits the max size.

@tclune
Copy link
Collaborator

tclune commented Jun 3, 2022

Probably should be replaced with radix-sort in any event.

@stale
Copy link

stale bot commented Aug 3, 2022

This issue has been automatically marked as stale because it has not had recent activity. If there are no updates within 7 days, it will be closed. You can add the "long term" tag to prevent the Stale bot from closing this issue.

@stale stale bot added the ❄️ Stale This issue has been marked stale label Aug 3, 2022
@mathomp4 mathomp4 added the ⌛ Long Term Long term issues label Aug 3, 2022
@stale stale bot removed the ❄️ Stale This issue has been marked stale label Aug 3, 2022
@mathomp4
Copy link
Member

mathomp4 commented Aug 3, 2022

Marking as long-term

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 Diff The changes in this pull request have verified to be zero-diff with the target branch. ⌛ Long Term Long term issues 🚫 Contingent - DNA Do Not Approve (DNA). These changes are contingent on other PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants