etaler / PyEtaler

The offical Python binding for Etaler via cppyy
BSD 3-Clause "New" or "Revised" License
10 stars 1 forks source link

Calling constructor with named parameters always calls the default constructor #2

Closed marty1885 closed 4 years ago

marty1885 commented 4 years ago

This is a cppyy bug.

When every a cppyy object is constructed with named parameters, the default constructor is called.

ex:

sp = et.SpatialPooler(input_shape=(256, ), output_shape=(32, ))

Then et::SpatialPooler::SpatialPooler() instead of et::SpatialPooler::SpatialPooler(Shape, Shape) is called. Thus the object is not constructed correctly.

wlav commented 4 years ago

Use of keyword arguments should have raised a TypeError as they are not yet supported. I'll see whether I can reproduce it.

marty1885 commented 4 years ago

@wlav Thanks for taking interest in the issue. The problem might relate to the fact that I'm running cppyy on ARM (Nvidia Xavier to be specific, since all my x86 machines have to run ROOT).

wlav commented 4 years ago

No, was simply a logic error that had crept in over time (there is/was no test checking the raising of TypeError). That's now fixed, but I think I'm simply going to implement keyword arguments as the original motivation for not doing that has become moot: it's not technically difficult, just that formal argument names have no meaning in C++ so there's no reason to assume they will be maintained as part of an interface, so I'm not in favor of keyword arguments.

Aside, since you also run ROOT, are you running cppyy from PyPI (or conda) or from pyroot_experimental?

marty1885 commented 4 years ago

That's now fixed, but I think I'm simply going to implement keyword arguments

Thanks a lot and good to know (thanks again)! I didn't see corresponding commits over at bitbucket tho, Will the patches be available later?

are you running cppyy from PyPI (or conda) or from pyroot_experimental?

I'm using cppyy from PyPI and ROOT from Arch Linux's repository.

wlav commented 4 years ago

Commit for getting a TypeError is here: https://bitbucket.org/wlav/cpycppyy/commits/f25f80a1043f40decadf71d9baae65e3c74323b0

Am still working on keyword arguments; will be in later.

wlav commented 4 years ago

Basics are in: https://bitbucket.org/wlav/cpycppyy/commits/c246c2a735d483a48b0c3c9edb955d8156620a81

This is for methods (which includes constructors). Still need to add the calls for functions and write more tests to make sure that the error handling is proper, but your specific case now works for me (incl. the implicit conversions from tuple through the initializer_list). See test growing here: https://bitbucket.org/wlav/cppyy/src/fa6d8effbb06ddc31067817e756c96ec49298cc5/test/test_pythonify.py#lines-395

I'll finish it up tomorrow and document it next release.

wlav commented 4 years ago

All seems to work for me. I'll cut the next release in a week or so.

marty1885 commented 4 years ago

@wlav Wow, thank you! The quick response and coding skills are amazing. You are amazing.

marty1885 commented 4 years ago

The issue should be resolved my the next cppyy release. This issule will be closed once cppyy updates and unit tests are run.

marty1885 commented 4 years ago

Thanks! Everything works!

Thank you. :+1: