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

Add python wrapper module #4

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

player1537
Copy link

@player1537 player1537 commented Oct 14, 2017

Not ready to merge

Still needs some changes before we can merge, but getting it out there to start the discussion and share the code. When we do merge, I can fix the merge conflict.

Description

This pull request adds a Python wrapper to PBNJ that is 1-1 with the original API (aka it's not very Pythonic and could be cleaned up a lot, but this gives the base Python module). The bulk of this builds on top of SWIG, which can auto-generate a Python wrapper around code in C/C++, and only required a couple of small changes to the C++ code.

In order to test everything out, first you need to make sure to have OSPRay, Embree, and PBNJ installed normally (i.e. all of them sudo make installed). Then, you need to generate the bindings using SWIG, from the root of the repo (not build).

Left to do

I don't think this is quite ready to merge, because there's still some quality-of-life changes that need to be made. In no particular order:

  1. Generating the swig bindings should be automatic and included in the CMake file. I don't know CMake well enough to do it, but I can give an equivalent Makefile to base the CMake one off of, just let me know.

  2. While compiling PBNJ, it should go ahead and create a Python egg/wheel so that pip installing the code is easier. If we ever want to push this to PyPI, then we'll need this.

  3. A lot of the contact/documentation stuff in the setup.py is very oversimplified. We should change it to be the most up-to-date (for instance, we probably don't want me listed as the point of contact and instead use something @seelabutk or similar).

  4. Ideally, the pbnj.i file (that SWIG uses to generate all the Python stuff) should be located either: in the main source tree or in its own directory. i.e. either src/pbnj.i or python/pbnj.i. I think the setup.py should still stay at the root though.

Testing

$ swig -c++ -python pbnj.i

Then you can pip install the module:

$ pip install --verbose .

And if all goes well, then you should now be able to run a test script, like the following:

with open("/tmp/config.json", "w") as f:
    f.write("""\
{
  "outputImageFilename": "temp.png",
  "imageSize": [512, 512],
  "cameraPosition": [-400, -20, -60],
  "filename": "/home/tanner/src/tapestry/examples/data/teapot.raw",
  "dimensions": [256, 256, 178],
  "dataset": "magnetic",
  "opacityAttenuation": 0.6,
  "backgroundColor": [0, 0, 0],
  "colorMap": "magma"
}
""")

import pbnj

pbnj.init()
config = pbnj.Configuration("/tmp/config.json")
camera = pbnj.Camera(config.imageWidth, config.imageHeight)
volume = pbnj.Volume(config.dataFilename, config.dataVariable, config.dataXDim,
                     config.dataYDim, config.dataZDim, True)
volume.setColorMap(config.colorMap)
volume.setOpacityMap(config.opacityMap)
volume.attenuateOpacity(config.opacityAttenuation)
renderer = pbnj.Renderer()
renderer.setVolume(volume)
renderer.setBackgroundColor(config.bgColor)
renderer.setCamera(camera)
camera.setPosition(config.cameraX, config.cameraY, config.cameraZ)
camera.setUpVector(0, 1, 0)
camera.centerView()
renderer.renderImage('foo.png')

@ahota
Copy link
Contributor

ahota commented Oct 14, 2017

This is great, thanks! The script at the bottom is already looking really nice. I can see what you mean by it not being very Pythonic. A couple ideas (not sure if possible in SWIG):

  • import pbnj does pbnj.init()
  • kwargs for the different object constructors, e.g. pbnj.Volume(dataFilename, dataVariable, dataXDim, dataYDim, dataZDim, mmap=True, colorMap=colorMap, opacityMap=opacityMap, ... ) instead of the current requirement of incrementally adding parameters

The latter would drastically reduce the length or "effort" needed for a script, I think.

Regarding CMake, it looks like it natively supports SWIG as of version 3.0.2. SWIG's documentation gives a simple example: http://www.swig.org/Doc1.3/Introduction.html#Introduction_build_system, and CMake's docs show FindSWIG is a builtin module (which is really nice): https://cmake.org/cmake/help/v3.0/module/FindSWIG.html. We currently target CMake 3.1 as the minimum version, so this should work.

@player1537
Copy link
Author

player1537 commented Oct 14, 2017 via email

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

Successfully merging this pull request may close these issues.

2 participants