1. Mike Miller
  2. pytave
  3. Pull requests

Pull requests

#24 Merged at 77af526
Repository
Deleted repository
Branch
default (b4e56f7255f7)
Repository
pytave
Branch
default

Refactor code and add option to specify namespace for pyexec and pyeval (fixes issue #25)

Author
  1. Abhinav Tripathi
Reviewers
Description

Refactor code, have common functions in pytave_utils

  • pytave_utils.{h,cc}: Have the 2 functions to get builtins module and an object from python as utils.
  • Makefile.am: Add pytave_utils{h,cc} to the build process.
  • pycall.cc: Use pytave_utils to get FUNC from python and remove unused code.
  • python_to_octave.cc: Use pytave_utils to get the builtins module

Add option to specify namespace when calling pyexec and pyeval (fixes issue #25)

  • pyeval.cc, pyexec.cc: Use the second parameter (if supplied) as namespace to eval/exec the python code.

Comments (21)

  1. Abhinav Tripathi author

    While working on this. I had 2 options, either to put the function in pytave.cc and create a new pytave.h (for including in other .cc files) or put them in a new file. I chose the second way (as I though the first one might require more changes in Makefile). Also, I moved the builtins_module thing to a separate function as it was required at many places.

    Currently this doesn't work. I get and undefined symbol error:

    >> pyexec("ll=1")
    error: /home/lucifer/work/pytave_main/pyexec.oct: failed to load: /home/lucifer/work/pytave_main/pyexec.oct: undefined symbol: _ZN6pytave22get_object_from_pythonERK12octave_valueRN5boost6python3api6objectE
    

    I thought that adding pytave_utils.h in Makefile.am should do the trick. I did a make clean also rm Makefile and then did configure and make.
    Mike Miller Colin Macdonald Please provide some suggestions to resolve the error. (I have very less knowledge of makefiles so my mistake might be pretty silly)

  2. Mike Miller repo owner

    Can you separate this into two commits, or maybe even two PRs? The majority of this seems to be about refactoring some code into common functions, that should be a separate commit.

    And the reason for the missing symbol error is that you have not compiled the new .cc file into anything. It should be added to COMMON_SOURCE_FILES.

    1. Abhinav Tripathi author

      Can you separate this into two commits, or maybe even two PRs?

      Sorry. I didn't do this till now because it will change after PR #25 is merged. And I thought that was more priority so I have left it for now. Will get back to it after that.

      you have not compiled the new .cc file into anything. It should be added to COMMON_SOURCE_FILES.

      Thanks I tried and it worked.

      Although many weird things are happening (namespaces are not working as desired), I have updated the PR. I will rebase and work on it more after #25 is merged.

  3. Abhinav Tripathi author

    In the last test in pyeval.cc I have removed sys from the global namespace. So even if user is using sys before running the test, he will have to re import sys after running the tests. I am assuming that this is not an issue. The last test is necessary as in symbolic we wish to import everything in a new namespace (I think Colin meant this on the issue #25).

    I am writing this comment here as I wrote it on the file but I had to do strip (to add 1 more test) which caused the comment to not be shown anymore (although there is "1" displayed next to the message icon in the Files Changed section).

  4. Mike Miller repo owner

    LGTM overall. Still needs some refining:

    • doc strings for pyeval and pyexec need updates to show the new usage
    • please add a nargin check to pyeval, it has been missing since the beginning
    • all new files need a file header with copyright and license statement
    • not a fan of the bool return value to indicate success or failure to parse the argument, how about returning a null or None object?
    • spacing and indentation errors pointed out inline below
    1. Abhinav Tripathi author

      not a fan of the bool return value to indicate success or failure to parse the argument, how about returning a null or None object?

      Well, I don't think object can have NULL or nullptr as value, can it? And setting it None may result in conflict since it can be used to get any object from python, even 'None'... I'll try to think of something else for defining success/failure..

    1. Mike Miller repo owner

      It looks to me like you squashed the return value change into the wrong commit. The first commit in this PR adds the new functions, with a bool return value. Then the second commit, which is supposed to be able adding the namespace option, changes it to void and checks for a None object.

      1. Abhinav Tripathi author

        Sorry for that. I don't actually know how to squash commits in hg. I just stripped the last changeset and committed everything together...

        I have fixed it now...

  5. Mike Miller repo owner

    I am planning on merging this as-is, but I have to ask, is there a reason to support passing the local namespace as either an identifier name or an object? I see the tests for both, but is there a valid use case for passing the namespace as an identifier of something in the main global namespace? I would like to clean up some redundancy, and I would rather just get rid of this case now if it's not important.

    1. Abhinav Tripathi author

      Well, it just that I re-used the pycall syntax. If the user has a dictionary as @pyobject (returned from some function) it would be convenient for him to just pass it instead of first storing it and then passing the variable name.
      But, I guess the more important use-case will be passing the namspace as string because instead of keeping track of the @pyobject, I would prefer creating a global dictionary and then always using that via it's name (although now that I see the tests, there is only 1 test that uses dictionary name). . If you wish, you (or I) may remove the case for passing the object for namespace...

      1. Colin Macdonald

        I guess the more important use-case will be passing the namspace as string

        I disagree. Most common use case should be passing in a py.dict: anything else is an anti-pattern. We should discourage it, same as we discouraging pyexec in user code.

        I don't see a strong reason for supporting a string input. Possibly something like Symbolic wants to create a namespace and re-use it: then it should keep it in an Octave persistent variable.

        1. Mike Miller repo owner

          Thank you, good, that's what I thought. I wrote the doc string to only mention that the argument should be a py.dict. I will clean up this misfeature or happily accept a PR that does so.