bugGNU Octave - Bugs: bug #56594, [Patch] containers.Map does not...

 
 

bug #56594: [Patch] containers.Map does not auto-convert differing numeric key-types

Submitter:  Markus Ebner <seijikun>
Submitted:  Mon 08 Jul 2019 08:40:22 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 20 Sep 2019 10:29:03 PM UTC, comment #6: 

Thank you.  I checked in the patch here https://hg.savannah.gnu.org/hgweb/octave/rev/170d05ce158f.

Marking as fixed and closing report.

Rik <rik5>
Group administrator
Fri 20 Sep 2019 10:52:07 AM UTC, comment #5: 

Pantxo and Rik: thanks for the reminder. Is the attached patch formatted as you expect?

(file #47552)

Guillaume <gyom>
Thu 19 Sep 2019 06:18:42 PM UTC, comment #4: 

@Guillaume: This looks like a good patch.  Can you make it in to a full changeset with commit message so it can be pushed directly?

Rik <rik5>
Group administrator
Fri 06 Sep 2019 03:57:06 PM UTC, comment #3: 

@Guillaume: Could you produce a proper patch file with a change log so that I can push it directly?

Pantxo Diribarne <pantxo>
Group Member
Tue 16 Jul 2019 11:01:21 AM UTC, comment #2: 

Here is a new version of the patch that fixes further issue with concatenation when the KeyTypes are not identical:


diff -r e3d886685813 scripts/+containers/Map.m
--- a/scripts/+containers/Map.m        Thu Jul 11 15:46:18 2019 +0200
+++ b/scripts/+containers/Map.m        Tue Jul 16 12:00:05 2019 +0100
@@ -448,9 +448,17 @@
       ## When concatenating maps, the data type of all values must be
       ## consistent with the ValueType of the leftmost map.
       keySet = cell (1, 0);
+      keyTypes = cell (1, 0);
       for i = 1:numel (varargin)
         keySet = [keySet, keys(varargin{i})];
+        keyTypes = [keyTypes, varargin{i}.KeyType];
       endfor
+      if (numel (unique (keyTypes)) != 1)
+        if (any (strcmp (keyTypes, "char")))
+          error ("containers.Map: cannot concatenate maps with numeric and string keys");
+        endif
+        keySet = cellfun (@(x) feval (keyTypes{1}, x), keySet, "UniformOutput", false);
+      endif
       valueSet = cell (1, 0);
       for i = 1:numel (varargin)
         valueSet = [valueSet, values(varargin{i})];
@@ -485,6 +493,8 @@
         else
           keys = cell2mat (keys);
         endif
+      elseif (! isa (keys, this.KeyType))
+        keys = feval (this.KeyType, keys);
       endif
       keys = num2hex (keys);  # Force to char matrix with single logical column
       if (cell_input)
@@ -725,6 +735,34 @@
 %!   assert (m.keys (), {key});
 %! endfor

+## Test using mixed numerical keys (subsref)
+%!test
+%! key = [1, 2, 3];
+%! val = {"One", "Two", "Three"};
+%! types = {"double", "single", "int32", "uint32", "int64", "uint64", ...
+%!          "int8", "uint8", "int16", "uint16"};
+%! for type1 = types
+%!   type = type1{1};
+%!   k = feval (type, key);
+%!   m = containers.Map (k, val);
+%!   for type2 = [types, "logical"]
+%!     type = type2{1};
+%!     k = feval (type, key(1));
+%!     assert (m(k), "One");
+%!   endfor
+%! endfor
+
+## Test using mixed numerical keys (subsasgn)
+%!test
+%! key = [1, 2, 3];
+%! val = {"One", "Two", "Three"};
+%! m = containers.Map(key, val);
+%! m (uint32 (1)) = "Four";
+%! assert (m.Count, uint64(3));
+%! assert (keys (m), {1, 2, 3});
+%! assert (m(1), "Four");
+%! assert (m(uint16 (1)), "Four");
+
 ## Test sort order of keys and values
 %!test
 %! key = {"d","a","b"};
@@ -750,6 +788,13 @@
 %! k = keys (m3);
 %! assert (numel (k), 2);
 %! assert (k, {"a", "b"});
+%! m1 = containers.Map (1, 1);
+%! m2 = containers.Map (single ([2, 3]), {2, 3});
+%! m3 = [m1, m2];
+%! assert (m3.KeyType, "double");
+%! assert (keys (m3), {1, 2, 3});
+%! m3 = [m2, m1];
+%! assert (m3.KeyType, "single");

 ## Test input validation
 %!error containers.Map (1,2,3)
@@ -805,3 +850,7 @@
 %! m1 = containers.Map ("KeyType", "cell", "ValueType", "any");
 %!error <unsupported ValueType>
 %! m1 = containers.Map ("KeyType", "char", "ValueType", "cell");
+%!error
+%! m1 = containers.Map (1, 1);
+%! m2 = containers.Map ("a", 2);
+%! m3 = [m1, m2];


For the very last test, I couldn't specify the expected error message:


Expected <cannot concatenate maps with numeric and string keys>, but got <containers.Map/horzcat method failed>


Guillaume <gyom>
Mon 15 Jul 2019 01:29:53 PM UTC, comment #1: 

Thanks Markus, it seems that this was an oversight and the conversion was only taking place when the keys were provided in a cell array. I modified your patch to make the change in "encode_keys" and use "feval" instead of "cast" (as the former is used a few lines above - not sure what is best). I also added some more tests.
This made me notice another issue with "vertcat" where the KeyType of the new object is not dealt with specifically.


--- a/scripts/+containers/Map.m Sat Jul 13 15:24:57 2019 -0700
+++ b/scripts/+containers/Map.m Mon Jul 15 14:22:52 2019 +0100
@@ -485,6 +485,8 @@
         else
           keys = cell2mat (keys);
         endif
+      elseif (! isa (keys, this.KeyType))
+        keys = feval (this.KeyType, keys);
       endif
       keys = num2hex (keys);  # Force to char matrix with single logical column
       if (cell_input)
@@ -725,6 +727,34 @@
 %!   assert (m.keys (), {key});
 %! endfor

+## Test using mixed numerical keys (subsref)
+%!test
+%! key = [1, 2, 3];
+%! val = {"One", "Two", "Three"};
+%! types = {"double", "single", "int32", "uint32", "int64", "uint64", ...
+%!          "int8", "uint8", "int16", "uint16"};
+%! for type1 = types
+%!   type = type1{1};
+%!   k = feval (type, key);
+%!   m = containers.Map (k, val);
+%!   for type2 = [types, "logical"]
+%!     type = type2{1};
+%!     k = feval (type, key(1));
+%!     assert (m(k), "One");
+%!   endfor
+%! endfor
+
+## Test using mixed numerical keys (subsasgn)
+%!test
+%! key = [1, 2, 3];
+%! val = {"One", "Two", "Three"};
+%! m = containers.Map(key, val);
+%! m (uint32 (1)) = "Four";
+%! assert (m.Count, uint64(3));
+%! assert (keys (m), {1, 2, 3});
+%! assert (m(1), "Four");
+%! assert (m(uint16 (1)), "Four");
+
 ## Test sort order of keys and values
 %!test
 %! key = {"d","a","b"};


Guillaume <gyom>
Mon 08 Jul 2019 08:40:22 PM UTC, original submission:  

There is code in the Octave containers.Map implementation, that compares types by taking into account, that they are numerics. But the key is then not converted into the actual numerical type, which is stored within the Map. This leads to different hashes and the key is therefore not found.

The following code-sample works in Matlab:

key = double([1,2,3]);
val = {"One", "Two", "Three"};
m = containers.Map(key, val);
m(uint32(1))


But fails in Octave with:

error: containers.Map: specified key <1> does not exist
error: called from
    subsref at line 391 column 13


Adding something to a map with a different numerical key-type seems to reinterpret the type, which leads to funny keys:

key = double([1,2,3]);
val = {"One", "Two", "Three"};
m = containers.Map(key, val);
m(uint32(1)) = "Four";
m.keys
ans =
{
  [1,1] =   2.1220e-314
  [1,2] =  1
  [1,3] =  2
  [1,4] =  3
}




My attempt at fixing this bug:

diff --git a/scripts/+containers/Map.m b/scripts/+containers/Map.m
index a0bcf3b..191ac5f 100644
--- a/scripts/+containers/Map.m
+++ b/scripts/+containers/Map.m
@@ -386,7 +386,7 @@ classdef Map < handle
                                         || ! isscalar (key))))
             error ("containers.Map: specified key type does not match the type of this container");
           endif
-          enckey = encode_keys (this, key);
+          enckey = encode_keys (this, cast (key, this.KeyType));
           if (! isfield (this.map, enckey))
             error ("containers.Map: specified key <%s> does not exist",
                    strtrim (disp (key)));
@@ -423,7 +423,7 @@ classdef Map < handle
             endif
             val = feval (this.ValueType, val);
           endif
-          key = encode_keys (this, key);
+          key = encode_keys (this, cast (key, this.KeyType));
           if (isfield (this.map, key))
             this.map.(key) = val;
           else
@@ -722,6 +722,22 @@ endclassdef
 %!   assert (m.keys (), {key});
 %! endfor

+## Test using mixed numerical keys (subsref)
+%!test
+%! key = double([1,2,3]);
+%! val = {"One", "Two", "Three"};
+%! m = containers.Map(key, val);
+%! assert(m(uint32(1)), 'One');
+
+## Test using mixed numerical keys (subsasgn)
+%!test
+%! key = double([1,2,3]);
+%! val = {"One", "Two", "Three"};
+%! m = containers.Map(key, val);
+%! m(uint32(1)) = 'Four';
+%! assert(m.Count, uint64(3));
+%! assert(keys (m), {1, 2, 3});
+
 ## Test sort order of keys and values
 %!test
 %! key = {"d","a","b"};


Markus Ebner <seijikun>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #47552:  bug56594.patch added by gyom (3KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by pantxo (Posted a comment)
  • -email is unavailable- added by gyom (Posted a comment)
  • -email is unavailable- added by seijikun (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-09-20 rik5 StatusPatch Reviewed Fixed
        Open/ClosedOpen Closed
    2019-09-20 gyom Attached File- Added bug56594.patch, #47552
    2019-09-19 rik5 StatusPatch Submitted Patch Reviewed
        Release5.1.0 dev
    2019-07-09 mtmiller StatusNone Patch Submitted

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code