Add "lexical" and "natural" sort strategies for notation codes #1205

osma · 2021-09-14T12:28:39Z

This PR makes it possible to select either "lexical" or "natural" sort strategy for sorting notation codes (see #937 for more background). The sorting is implemented both on the PHP and JS sides. The skosmos:sortByNotation configuration setting, which previously was just a boolean value, can now given a literal value to select the appropriate sort strategy:

skosmos:sortByNotation "false";    # disabled, i.e. sort by labels, not notations
skosmos:sortByNotation "lexical";  # lexical sorting
skosmos:sortByNotation "natural";  # natural sorting
skosmos:sortByNotation "true";     # default sorting i.e. decimal, as it's the most common

TODO:

more testing - ~~it seems that sorting by notation is not always working on the JS side because notations are missing for the jsTree concept objects~~ (fixed, the problem was in Finto layout footer.inc)
more unit tests to check that the sort order is as expected
update wiki documentation (Configuration)

Fixes #937

codecov · 2021-09-14T12:28:47Z

Codecov Report

Merging #1205 (e53e82b) into master (4b4359d) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #1205      +/-   ##
============================================
- Coverage     68.26%   68.24%   -0.03%     
- Complexity     1609     1613       +4     
============================================
  Files            32       32              
  Lines          3955     3958       +3     
============================================
+ Hits           2700     2701       +1     
- Misses         1255     1257       +2

Impacted Files	Coverage Δ
model/Concept.php	`80.95% <100.00%> (ø)`
model/ConceptProperty.php	`96.42% <100.00%> (-3.58%)`	⬇️
model/ConceptPropertyValue.php	`86.25% <100.00%> (-0.50%)`	⬇️
model/VocabularyConfig.php	`93.64% <100.00%> (+0.10%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b4359d...e53e82b. Read the comment docs.

osma · 2021-09-14T12:29:34Z

model/ConceptPropertyValue.php

@@ -33,12 +33,7 @@ public function __construct($model, $vocab, $resource, $prop, $clang = '')

    public function __toString()
    {
-        $label = is_string($this->getLabel()) ? $this->getLabel() : $this->getLabel()->getValue();
-        if ($this->vocab->getConfig()->sortByNotation()) {
-            $label = ltrim($this->getNotation() . ' ') . $label;


Adding the notation here doesn't seem to have any effect on the sort order so I just removed it.

osma · 2021-09-14T12:30:10Z

model/VocabularyConfig.php

     */
-    public function sortByNotation()
+    public function getSortByNotation(): ?string


I renamed the method for better consistency with other getters, and redefined its return value.

osma · 2021-09-14T12:31:29Z

resource/js/scripts.js

 // Natural sort from: http://stackoverflow.com/a/15479354/3894569
+// adapted to return only -1 or 1 using negVsPos function above


Looking at the documentation of the jsTree sort plugin, it expects that the return value of the sort function is either -1 or 1 (anything other than -1 will be interpreted as 1, AFAICT). To make sure that the return values meet those expectations, I added a coercing function.

…blems

osma · 2021-09-15T12:01:14Z

it seems that sorting by notation is not always working on the JS side because notations are missing for the jsTree concept objects

Finally found the cause of the missing notation information: the footer.inc customizations in the Finto layout were overriding key JS methods, and they were not in sync with changes that happened in the original Skosmos JS code (that they are based on) a long time ago. Fixed in this Finto-data commit. Now the notation information is available and sorting by notation code works (both for decimal and natural sort order) in the hierarchy tree.

However, it turns out that the natural sort order is a bit different for PHP vs JS. On the PHP side, numbers with leading zeros are always first (PHP uses Martin Pool's algorithm) The current JS implementation of naturalCompare ignores leading zeros. The differences can be seen in this screenshot, where the notation codes 33.03 and 33.04 are placed differently in the hierarchy sidebar (JS) and the concept view (PHP):

Not sure if this is a big issue - the example is a bit artificial, because YKL/PLC shouldn't use natural sorting but decimal, where this doesn't happen. And for vocabularies where natural sorting is preferred (i.e. PTVL), leading zeros probably shouldn't be used anyway.

osma · 2021-09-15T14:11:22Z

I added unit tests that verify the sort order is correct on the PHP side in both "decimal" and "natural" cases. Ready for review.

I will update the wiki documentation after merging.

kouralex · 2021-09-15T19:50:19Z

Looks great!

Three remarks, one that you already pinpointed:

Here (look at the image below) the natural order faces the same problem. I am not completely sure, if this vocabulary (HKLJ/HCLCS) should be using the "decimal" order. If that is the case (which may very well be), there is no problem as you mentioned and explained. (URL for your convenience: https://finto.fi/hklj/fi/page/001)

Here, a "real" problem where irregardless of the setting, this always shows up like this: (URL for your convenience: https://finto.fi/hklj/fi/page/691.1)

It seems like the sorting is done by the label only. For mapping properties one should probably also sort by its (if vocabulary known to Skosmos) sortByNotation setting. I am not sure if this complicates the issue a bit too much - maybe (I think) it is OK to use the same value.

Here it seems like the search is done using always the "decimal" order, even though one changes the value to "natural". This would mean that no vocabulary may use true "natural" sort strategy in search queries, am I right? The same naturally applies to the typeahead component, too (they use the same implementation under the hood).

kouralex

I already commented in the PR where I had three remarks (of which at least two require some action or explanation).

kouralex · 2021-09-15T20:08:33Z

As a fourth item, I also noticed that with "false" value only affects concept info part - it does not affect search nor JS (this may or may not be justified):

osma · 2021-09-16T06:53:34Z

Thanks for the review @kouralex . You're right, I didn't consider ordering of search results at all in this PR. I will take a closer look at that and the other things you mentioned.

…curate

osma · 2021-09-16T08:23:14Z

Renamed the "decimal" strategy to "lexical". It's not really appropriate for decimal numbers (e.g. "110" sorts before "13") but is just based on lexical string sorting. Amended the PR title and OP accordingly.

osma · 2021-09-16T10:03:45Z

Regarding the four comments:

1. Different natural sort orders in PHP and JS

Already discussed above, not a big issue IMHO.

2. Search result order

I looked at the ordering of search results by notation in more detail and opened an issue #1209. But I don't think there's anything worth changing in this area so I set that issue to the Blue Sky milestone.

3. Mapping property order

For mapping properties one should probably also sort by its (if vocabulary known to Skosmos) sortByNotation setting. I am not sure if this complicates the issue a bit too much - maybe (I think) it is OK to use the same value.

I opened a separate issue #1208 about ordering mapping property values. I think that's out of scope for this PR and issue.

4. Setting "false" has no effect on hierarchy

As a fourth item, I also noticed that with "false" value only affects concept info part - it does not affect search nor JS (this may or may not be justified):

You are right, I'm currently investigating this. I think this happens because when sorting by notation is disabled (set to false), the sorting of jsTree nodes (representing concepts) falls back to comparison of node text. But that text includes the notation code, so notation codes will affect the sorting anyway.

…d to sort by notation.

osma · 2021-09-16T10:31:57Z

I fixed the JS side sorting in the hierarchy when the setting is false. I already responded to the three other comments above.
Ready for another round of review and (hopefully) merging @kouralex

kouralex

Seems to be working now and the issues I pinpointed have been addressed. Great job @osma !

I noticed that the default value is "false" if the property is not set at all, which kind of makes sense, however, the comments in the code imply it could be "lexical" (which is true if the value is "true"!).

Apart from that, ready for merging! 🙂

osma · 2021-09-16T13:58:21Z

I fixed a minor JS issue and clarified the comments about default values. Will merge after tests have passed.

sonarcloud · 2021-09-16T13:58:38Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
5 Code Smells

No Coverage information
25.6% Duplication

"decimal" and "natural" sort strategies for notation codes. Fixes #927

3d4b80a

osma added the enhancement label Sep 14, 2021

osma added this to the 2.12 milestone Sep 14, 2021

osma self-assigned this Sep 14, 2021

osma commented Sep 14, 2021

View reviewed changes

Revert (partially) refactoring of showNotation, as it could cause pro…

651a5fe

…blems

osma mentioned this pull request Sep 14, 2021

The way concepts with notation codes are sorted should be described in config.ttl #937

Closed

osma added 2 commits September 15, 2021 16:56

Remove tests for functionality that was removed

499a121

Add tests for decimal and natural sorting of notations

dbd4dc7

osma marked this pull request as ready for review September 15, 2021 13:59

osma requested a review from kouralex September 15, 2021 14:11

kouralex requested changes Sep 15, 2021

View reviewed changes

rename "decimal" notation sort strategy to "lexical" which is more ac…

65714de

…curate

osma changed the title ~~Add "decimal" and "natural" sort strategies for notation codes~~ Add "lexical" and "natural" sort strategies for notation codes Sep 16, 2021

osma mentioned this pull request Sep 16, 2021

Sort mapping concepts by notation code #1208

Open

Better sorting of hierarchy nodes, ignoring notation unless configure…

4b8a7c9

…d to sort by notation.

osma requested a review from kouralex September 16, 2021 10:32

kouralex approved these changes Sep 16, 2021

View reviewed changes

osma added 2 commits September 16, 2021 16:53

Add var declaration (bug found by sonarqube)

163f16a

Clarify comments about default values

e53e82b

osma merged commit 794bd18 into master Sep 16, 2021

osma deleted the issue937-sort-by-notation-strategy branch September 16, 2021 14:03

This was referenced Sep 16, 2021

Clicking on notation code in hierarchy loads wrong page #1210

Closed

Hierarchy view shows classes in wrong order #861

Closed

osma mentioned this pull request Oct 26, 2022

Opening the hierarchy for a concept takes a long time #1377

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "lexical" and "natural" sort strategies for notation codes #1205

Add "lexical" and "natural" sort strategies for notation codes #1205

osma commented Sep 14, 2021 •

edited

Loading

codecov bot commented Sep 14, 2021 •

edited

Loading

osma Sep 14, 2021

osma Sep 14, 2021

osma Sep 14, 2021

osma commented Sep 15, 2021

osma commented Sep 15, 2021

kouralex commented Sep 15, 2021

kouralex left a comment

kouralex commented Sep 15, 2021 •

edited

Loading

osma commented Sep 16, 2021

osma commented Sep 16, 2021

osma commented Sep 16, 2021 •

edited

Loading

osma commented Sep 16, 2021

kouralex left a comment

osma commented Sep 16, 2021

sonarcloud bot commented Sep 16, 2021

		// Natural sort from: http://stackoverflow.com/a/15479354/3894569
		// adapted to return only -1 or 1 using negVsPos function above

Add "lexical" and "natural" sort strategies for notation codes #1205

Add "lexical" and "natural" sort strategies for notation codes #1205

Conversation

osma commented Sep 14, 2021 • edited Loading

codecov bot commented Sep 14, 2021 • edited Loading

Codecov Report

osma Sep 14, 2021

Choose a reason for hiding this comment

osma Sep 14, 2021

Choose a reason for hiding this comment

osma Sep 14, 2021

Choose a reason for hiding this comment

osma commented Sep 15, 2021

osma commented Sep 15, 2021

kouralex commented Sep 15, 2021

kouralex left a comment

Choose a reason for hiding this comment

kouralex commented Sep 15, 2021 • edited Loading

osma commented Sep 16, 2021

osma commented Sep 16, 2021

osma commented Sep 16, 2021 • edited Loading

1. Different natural sort orders in PHP and JS

2. Search result order

3. Mapping property order

4. Setting "false" has no effect on hierarchy

osma commented Sep 16, 2021

kouralex left a comment

Choose a reason for hiding this comment

osma commented Sep 16, 2021

sonarcloud bot commented Sep 16, 2021

osma commented Sep 14, 2021 •

edited

Loading

codecov bot commented Sep 14, 2021 •

edited

Loading

kouralex commented Sep 15, 2021 •

edited

Loading

osma commented Sep 16, 2021 •

edited

Loading