hold targeter sorts proximities alphabetically, not numerically

[Galen Charlton]

When the hold targeter calculates the list of proximities of eligible copies from the hold's pickup library, it then looks for capturable copies that are closest by sorting by ascending proximity...

ASCIIbetically.

This can lead to exactly the wrong outcome, especially if org unit proximity adjustments are in use. For example, if a proximity ends up being adjusted up to (say) 15 or 100 to discourage associated copies from being targeted, since "15" < "2" (or "4", etc.), those copies end up being preferred over ones whose proximity value is numerically lower.

Evergreen 2.7+

[Josh Stompro (u-launchpad-stompro-org)]

I can confirm that we are seeing this problem on 2.8.4.

We wanted a hard priority order for all our branches in two system, and we were using a 100 point bump for cross system hold proximity.

The targeter was first targeting copies at 100+ level proximity.

Josh - Lake Agassiz Regional Library

[Josh Stompro (u-launchpad-stompro-org)]

Example of what we are seeing, it is sorting like this.

select * from action.hold_copy_map where hold=169510 order by proximity::text;
   id | hold | target_copy | proximity
--------+--------+-------------+-----------
 604706 | 169510 | 2914854 | 1
 604703 | 169510 | 2929823 | 10
 604697 | 169510 | 2933092 | 105
 604696 | 169510 | 2950820 | 106
 604695 | 169510 | 2949622 | 106
 604698 | 169510 | 2917079 | 107
 604699 | 169510 | 2933059 | 107
 604691 | 169510 | 2951885 | 108
 604693 | 169510 | 2940831 | 109
 604692 | 169510 | 2938076 | 109
 604688 | 169510 | 2944898 | 110
 604687 | 169510 | 2915112 | 110
 604694 | 169510 | 2956799 | 111
 604689 | 169510 | 2920644 | 112
 604690 | 169510 | 2921416 | 112
 604702 | 169510 | 2934336 | 12
 604704 | 169510 | 2928317 | 13
 604685 | 169510 | 2942927 | 14
 604707 | 169510 | 2948626 | 4
 604708 | 169510 | 2950746 | 4
 604700 | 169510 | 2923177 | 5
 604705 | 169510 | 2939696 | 6
 604686 | 169510 | 2914459 | 7
 604701 | 169510 | 2920388 | 9

Not like this
select * from action.hold_copy_map where hold=169510 order by proximity;
   id | hold | target_copy | proximity
--------+--------+-------------+-----------
 604706 | 169510 | 2914854 | 1
 604708 | 169510 | 2950746 | 4
 604707 | 169510 | 2948626 | 4
 604700 | 169510 | 2923177 | 5
 604705 | 169510 | 2939696 | 6
 604686 | 169510 | 2914459 | 7
 604701 | 169510 | 2920388 | 9
 604703 | 169510 | 2929823 | 10
 604702 | 169510 | 2934336 | 12
 604704 | 169510 | 2928317 | 13
 604685 | 169510 | 2942927 | 14
 604697 | 169510 | 2933092 | 105
 604696 | 169510 | 2950820 | 106
 604695 | 169510 | 2949622 | 106
 604698 | 169510 | 2917079 | 107
 604699 | 169510 | 2933059 | 107
 604691 | 169510 | 2951885 | 108
 604693 | 169510 | 2940831 | 109
 604692 | 169510 | 2938076 | 109
 604688 | 169510 | 2944898 | 110
 604687 | 169510 | 2915112 | 110
 604694 | 169510 | 2956799 | 111
 604690 | 169510 | 2921416 | 112
 604689 | 169510 | 2920644 | 112

I think the culprit must be line 1992 of
http://git.evergreen-ils.org/?p=Evergreen.git;a=blob;f=Open-ILS/src/perlmods/lib/OpenILS/Application/Storage/Publisher/action.pm;hb=HEAD#l1992

   for my $p ( sort keys %$prox_list ) {

Changing it to
   for my $p ( sort {$a<=>$b} keys %$prox_list ) {

On a test system appears to correct the issue.

There is another instance of sorting the proximity that probably needs to be fixed also. I'm not sure what it is doing, but a bad sort probably isn't good.
http://git.evergreen-ils.org/?p=Evergreen.git;a=blob;f=Open-ILS/src/perlmods/lib/OpenILS/Application/Storage/Publisher/action.pm;hb=HEAD#l1436

Josh

[Josh Stompro (u-launchpad-stompro-org)]

I wonder if the following is also off, sorting the best hold selection sorting options by value using cmp, which is for string comparison. Since currently the number of options is less than 10, it acts correctly, but I wonder if it would break if two more sort options are added. Otherwise I don't think that the opportunistic capture is effected, since the sorting is done in the DB.

    ) || {
        pprox => 1, hprox => 8, aprox => 2, priority => 3,
        cut => 4, depth => 5, htime => 7, rtime => 6
    };

    # Return only the keys of our hash, sorted by value,
    # keys for null values omitted.
    return [
        grep { defined $row->{$_} } (
            sort {$row->{$a} cmp $row->{$b}} keys %$row
        )
    ];

Yep, I think that could be a problem in the future.
Here is a little test.

----
#!/usr/bin/perl

use Data::Dumper;

sub get_hold_sort_order {
my $row =
{
        pprox => 1, hprox => 8, aprox => 2, priority => 3,
        cut => 4, depth => 5, htime => 7, rtime => 6,
        taco => 9, foo => 10, bar => 11, baz => 12
};

    # Return only the keys of our hash, sorted by value,
    # keys for null values omitted.
    return [
        grep { defined $row->{$_} } (
            sort {$row->{$a} cmp $row->{$b}} keys %$row
        )
    ];
}

print Dumper(get_hold_sort_order());
-----

Output shows that 10,11,12 are sorted as the 2nd-4th sort options.
$VAR1 = [
          'pprox',
          'foo',
          'bar',
          'baz',
          'aprox',
          'priority',
          'cut',
          'depth',
          'rtime',
          'htime',
          'hprox',
          'taco'
        ];

}

using numeric comparison <=>

$VAR1 = [
          'pprox',
          'aprox',
          'priority',
          'cut',
          'depth',
          'rtime',
          'htime',
          'hprox',
          'taco',
          'foo',
          'bar',
          'baz'
        ];

[Josh Stompro (u-launchpad-stompro-org)]

Working branch at /user/stompro/lp1511828_targeter_proximity_sort_fix
http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/stompro/lp1511828_targeter_proximity_sort_fix

Does this need release notes?

How about tests? I only have a little experience with the PGtap tests which wouldn't help with this fix. Are there any current test for the hold targeting that could be expanded to have proximities above 9 to test for this situation in the future. If someone could give me some pointers I'll look into it.

Galen, thank you again for figuring this out.
Josh

[Bill Erickson (berick)]

While we're in this part of the code, we could also reduce Perl warnings in the logs (currently: "Use of uninitialized value in string comparison (cmp) at ...") by swapping the order of the sort + grep:

grep { defined $row->{$_} } (
       sort {$row->{$a} <=> $row->{$b}} keys %$row
)

becomes:

sort { $row->{$a} <=> $row->{$b} } (
     grep { defined $row->{$_} } keys %$row
)

[Josh Stompro (u-launchpad-stompro-org)]

Thanks Bill, I'll add that change also.

I would like to track down some of the other warnings that pop up when targeting also, I'm not sure I'll have a chance here though.

There are a few that come up when there are no holdable copies.

I don' t understand why this code even needs to be called at all if there are no holdable copies to get the minimum proximity of.

osrfsys.log:[2015-11-01 11:15:02] open-ils.storage [WARN:449:Application.pm:590:] open-ils.storage.action.hold_request.copy_targeter: Use of uninitialized value $min_prox in delete at /usr/local/share/perl/5.14.2/OpenILS/Application/Storage/Publisher/action.pm line 1441.

osrfsys.log:[2015-11-01 11:15:02] open-ils.storage [WARN:449:Application.pm:590:] open-ils.storage.action.hold_request.copy_targeter: Use of uninitialized value $min_prox in anonymous hash ({}) at /usr/local/share/perl/5.14.2/OpenILS/Application/Storage/Publisher/action.pm line 1441.

osrfsys.log:[2015-11-01 11:15:02] open-ils.storage [WARN:449:Application.pm:590:] open-ils.storage.action.hold_request.copy_targeter: Argument "" isn't numeric in sort at /usr/local/share/perl/5.14.2/OpenILS/Application/Storage/Publisher/action.pm line 1992.

[Galen Charlton (gmc)]

> Does this need release notes?

Not really -- it's not a new feature or enhancement, and the bugfix
won't require any special action to make use of.

> How about tests? I only have a little experience with the PGtap tests
> which wouldn't help with this fix. Are there any current test for the
> hold targeting that could be expanded to have proximities above 9 to
> test for this situation in the future. If someone could give me some
> pointers I'll look into it.

Alas, no. The hold targeter could definitely stand to have some test
cover under src/perlmods/live_t, but for the purpose of your patch, I
think it suffices to write some detailed testing notes for the commit
message.

Regards,

Galen
--
Galen Charlton
Infrastructure and Added Services Manager
Equinox Software, Inc. / The Open Source Experts
email: <email address hidden>
direct: [phone]
cell: [phone]
skype: gmcharlt
web: http://www.esilibrary.com/
Supporting Koha and Evergreen: http://koha-community.org &
http://evergreen-ils.org

[Bill Erickson (berick)]

On Mon, Nov 2, 2015 at 8:39 AM, Josh Stompro <email address hidden> wrote:

> Thanks Bill, I'll add that change also.
>
> I would like to track down some of the other warnings that pop up when
> targeting also, I'm not sure I'll have a chance here though.
>
> There are a few that come up when there are no holdable copies.
>
> I don' t understand why this code even needs to be called at all if
> there are no holdable copies to get the minimum proximity of.
>

Indeed, and choose_nearest_copy() does nothing if no copies are passed.

Maybe just:

- } else {
+ } elsif (defined $min_prox) {
                 $best = choose_nearest_copy($hold, { $min_prox =>
delete($$prox_list{$min_prox}) });
             }

[Josh Stompro (u-launchpad-stompro-org)]

Branch updated with warning removal fixes.

user/stompro/lp1511828_targeter_proximity_sort_fix

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/stompro/lp1511828_targeter_proximity_sort_fix

I've tested this on a test server and have also been running it in production. No more hold targeter warnings from action.pm and the sorting works correctly.

Josh

[Josh Stompro (u-launchpad-stompro-org)]

Did a force update to include testing notes.
Josh

[Galen Charlton (gmc)]

Tested and pushed to master, rel_2_9, rel_2_8, and rel_2_7. Thanks, Josh!