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

Tables as first argument introduces ambiguity - proposal: table wrappers classes #9

Open
gforge opened this issue May 9, 2016 · 3 comments

Comments

@gforge
Copy link

gforge commented May 9, 2016

I'm struggling to grasp how to approach argument that can either be a string or a table for the __init function. I figured that if I go with ordered arguments the argcheck should just use the env.istype and let me worry about the implementation details. Unfortunately nothing seems to happen when using the nonamed argument, here's an example:

local argcheck = require 'argcheck'

test = torch.class('test')
env = require 'argcheck.env' -- retrieve argcheck environement
env.istype = function(obj, typename)
  if (typename == "table|string") then
    return torch.type(obj) == "table" or
      torch.type(obj) == "string"
  end
  if (typename == "torch.*Tensor") then
    -- regular expressions don't work therefore this
    return torch.type(obj) == "torch.IntTensor" or
      torch.type(obj) == "torch.FloatTensor" or
      torch.type(obj) == "torch.DoubleTensor"
  end
  return torch.type(obj) == typename
end

test.__init = argcheck{
  nonamed=true,
  {name="self", type = "test"},
  {name="tbl", type = "table|string", default=false},
  call = function(self, tbl) -- called in case of success
    print(tbl)
   end
}

print("-- No arguments")
a = test.new()

print("\n-- String")
a = test.new("test")

print("\n-- Table")
a = test.new({1,2,3})

print("\n-- Should throw error")
a = test.new{tbl={1,2,3}}

Outputs:

-- No arguments 
false   

-- String   
test    

-- Table    
{
  1 : 1
  2 : 2
  3 : 3
}

-- Should throw error   
{
  tbl : 
    {
      1 : 1
      2 : 2
      3 : 3
    }
}

The documentation is a little thin on classes, if I'm missing something please consider extending the docs a little so that this awesome package gets easier to implement.

@soumith
Copy link
Member

soumith commented May 10, 2016

do any of the argcheck users know this? cc: @andresy @ajabri @lvdmaaten

@gforge
Copy link
Author

gforge commented May 10, 2016

If we have something other than a table it does work fine:

local check_init = argcheck{
  noordered=true,
  {name="tbl", type = "string", default=false}}
function test:__init(tbl) -- called in case of success
  tbl = check_init({tbl = tbl})
    print(tbl)
end

print("-- No arguments")
a = test.new()

print("\n-- String")
a = test.new("test")

print("\n-- Should throw error")
a = test.new{tbl="asdsa"}

Ends up with the expected output:

-- No arguments 
false   

-- String   
test    

-- Should throw error   
/home/max/tools/torch/install/bin/luajit: [string "argcheck"]:28: 
Arguments:

{
  [tbl = string]  --  [default=false]
}


Got: table={ tbl=table }
invalid arguments!
stack traceback:
    [C]: in function 'error'
    [string "argcheck"]:28: in function 'check_init'
    test.lua:23: in function '__init'
    /home/max/tools/torch/install/share/lua/5.1/torch/init.lua:91: in function 'new'
    test.lua:34: in main chunk
    [C]: in function 'dofile'
    ...ools/torch/install/lib/luarocks/rocks/trepl/scm-1/bin/th:145: in main chunk
    [C]: at 0x00406670

I guess this could be averted by specifying the expected table depth. This would cost quite a bit performance but in some use cases the performance isn't that important, e.g. in the torch-dataframe we want to allow users to provide a single column name or multiple column names.

The core problem is that the argument packing in Lua stuffs everything into the first parameter without leaving a trace. In my mind this is a design flaw as it unnecessarily limits Lua capabilities, ideally I would like that they add a hidden indicator of that this packing has occurred, e.g. a boolean like __packed_arg.

@gforge
Copy link
Author

gforge commented May 11, 2016

If this is unsolvable within the current Lua design (found this discussion) a possible (hacky) option could be to have custom classes, e.g. argcheck.Table, argcheck.Array, etc. that take a table as an argument which is then extracted once the variable has been identified. This should take care of any ambiguity cases and possibly also allow for some basic checks of the incoming tables.

@gforge gforge changed the title The nonamed doesn't affect behavior Tables as first argument introduces ambiguity - proposal: table wrappers classes Jul 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants