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

 
 

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

Submitted by:  Markus Ebner <seijikun>
Submitted on:  Mon 08 Jul 2019 08:40:22 PM UTC  
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Patch Submitted Assigned to:  None
Originator Name:  Open/Closed:  Open
Release:  5.1.0 Operating System:  GNU/Linux

Add a New Comment (Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

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>
Project 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:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -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)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

     

     

    Follows 1 latest change.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-07-09 mtmiller StatusNone => Patch Submitted

    Back to the top


    Powered by Savane 3.5