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

Integers are not sorted correctly #11023

Closed
FilipSzm opened this issue Jan 10, 2022 · 14 comments
Closed

Integers are not sorted correctly #11023

FilipSzm opened this issue Jan 10, 2022 · 14 comments
Assignees
Labels

Comments

@FilipSzm
Copy link

Description of the Issue

Negative numbers are sorted in reverse order

Steps to Reproduce the Issue

  1. Open attached file
    numbers.txt
  2. Click 'Edit' in toolbar
  3. Click 'Line Operations' in Edit menu
  4. Click 'Sort Lines As Integers Descending ' or 'Sort Lines As Integers Ascending ' in Line Operations submenu

Expected Behavior

Numbers are sorted ascending or descending (depending on option in step 4)

Actual Behavior

Non negative numbers are sorted correctly while negative numbers are sorted in reverse order (ascending for descending sort and vice versa)

Debug Information

Notepad++ v8.1.9.3 (64-bit)
Build time : Dec 6 2021 - 19:21:37
Path : C:\Program Files\Notepad++\notepad++.exe
Command Line :
Admin mode : OFF
Local Conf mode : OFF
Cloud Config : OFF
OS Name : Windows 10 Pro (64-bit)
OS Version : 2009
OS Build : 19042.1415
Current ANSI codepage : 1250
Plugins : mimeTools.dll NppConverter.dll NppExport.dll

@alankilborn
Copy link
Contributor

Maybe the menu choice should say it works on unsigned integers.
If you use the "Decimals" menu choice(s), it works.

@WinterSilence
Copy link

@alankilborn it's looks like bug

@lisareichelson
Copy link

Hi, can I work on this?

@alankilborn
Copy link
Contributor

Maybe it could be the user manual that needs work here. See HERE. @pryrt what do you think?

Or possibly that plus a menu text change, as I suggested before.

@ArkadiuszMichalski
Copy link
Contributor

If the name is integer, it should also sort negative ones in the correct order.

If the name will be natural number or counting numbers or non-negative integers or unsigned integers then this command should treat negative as positive, or omit such values and not change their order.

Currently it works as described on first post. Although, maybe that's the intended behavior? It looks like it sorts negative and positive separately, with a 0 in between, but negative in fact are treated as positive.

init      integers ascendings
-7        -3
-3        -5
-5        -7
1          0
0          1
4          2
2          4

@pryrt
Copy link
Contributor

pryrt commented Jan 19, 2022

Personally, I don't see the utility of an "integer sort" that sorts in the order -3/-5/-7/0/1/2/4. I would say it's a bug in the implementation. If the developers decide not to make a change, then I will update the documentation to match that decision, but I would assume it's a bug that should be fixed, rather than documented around.

@alankilborn
Copy link
Contributor

I'm sure it was never meant to sort negative integers, from the time of first implementation. Why? It's too obvious of a miss.
IMO, change its name (to reflect unsigned) or remove the sort entirely (as one of the others does it correctly anyway).

@alankilborn
Copy link
Contributor

then this command should treat negative as positive, or omit such values and not change their order.

I don't see much logic in that. It should be a precondition of the sort that you only have the correct type of values over the entire sorting range. If you don't, don't complain about unexpected behavior.

@ArkadiuszMichalski
Copy link
Contributor

ArkadiuszMichalski commented Jan 20, 2022

@donho This looks like regression introduced by #4413 in version 7.5.7. The previous versions work fine when we taken into account only numbers. Reading the issue, I don't see that the author's intention was to completley omit negative numbers.

If now it should be only for natural numbers, then this name should be corrected, and it is worth adding in the documentation how negative values are treated.

Edit: This change was probably done to make more flexible how we write numbers, even with text in front of them, and the sort was supposed to work fine anyway. But negative numbers are treated independently on this approach. Try with this values:

-7
-31
-3
-5
-51
a 11
1
0
4
2
a b 8
a 5
a b 3
a b c 1

@donho donho self-assigned this Jan 20, 2022
@sherwinnie
Copy link
Contributor

Hi, this is a student from University of Michigan. We found out this issue and it seems like the regression bug introduced by #4413 was made by a previous student from University of Michigan as a school project. We think we will be able to fix this bug and redeem the mistake. Is it okay if we claim this issue?

@sherwinnie
Copy link
Contributor

We have implemented a sorting algorithm that takes negative numbers into account. Essentially, it behaves like natural sort, but it also takes in dashes (-) for negative numbers.

However, there are an edge case that we want to ask the community what the intended behavior should be.

Suppose we want to compare "-01" and "-1". Numerically, they have the same value. Then, which one should take priority? Should it be sorted ascendingly to:

-01
-1

or

-1
-01

We think the second option makes more sense, since the numerical value tied, we should compare them as two strings, in which case, the longer string has higher priority.

@rdipardo
Copy link
Contributor

rdipardo commented Apr 6, 2022

I'm pretty sure ASCII character order is how you decide those things:

-01
-1

Any rational file system would do the same:

$ tree --sort=name
.
├── -01.txt
└── -1.txt

@sherwinnie
Copy link
Contributor

I'm pretty sure ASCII character order is how you decide those things:

-01
-1

Any rational file system would do the same:

$ tree --sort=name
.
├── -01.txt
└── -1.txt

I see. Thank you!

@donho
Copy link
Member

donho commented Apr 10, 2022

Tested PR #11481, it solved also issue #2025:

2 3
1 4
2 -10
-2 20

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

Successfully merging a pull request may close this issue.

9 participants