hyperion-rt / hyperion

Hyperion Radiative Transfer Code
http://www.hyperion-rt.org
BSD 2-Clause "Simplified" License
52 stars 26 forks source link

Implementation of Voronoi RT #92

Closed astrofrog closed 10 years ago

astrofrog commented 10 years ago

@bluescarni - just for info, this is the PR with all the functionality we've worked on so far (makes it easier to review the whole thing).

astrofrog commented 10 years ago

Note to self: need to disable the PDA for this type of grid. Need to not define cell_width for example.

astrofrog commented 10 years ago

@bluescarni - let's see if the tests pass here!

bluescarni commented 10 years ago

@astrofrog mhmm looks like the c++ compiler is not found?

astrofrog commented 10 years ago

@bluescarni - I've tried to add a line to .travis.yml to get it to install the C++ compiler

bluescarni commented 10 years ago

@astrofrog seems like it is not enough yet. How about adding the g++ compiler explicitly a-la sudo apt-get install g++? Otherwise it would seem some kind of problem on the Python side - to be pedantic it should not try to call gcc, but g++ when compiling c++ code.

astrofrog commented 10 years ago

@bluescarni - ok, trying that :)

bluescarni commented 10 years ago

@astrofrog thanks, this looks better. It fails now in the tests when it tries to unpickle the neighbours tables computed with qhull and used for sanity checks.

My understanding is that the class OrderedDict, used by pickle, was introduced in python 2.7 but the travis instance has python 2.6. Maybe I could store the tables for comparison in another format readable by older Python versions?

astrofrog commented 10 years ago

@bluescarni - it might be good to store it in a different format, since pickles don't really have any guarantees on long-term storage. Can you see if there is a different way of storing it as e.g. an ASCII table or FITS table?

astrofrog commented 10 years ago

Alternatively, you could store it as an HDF5 table (astropy's Table.write can write to hdf5)

astrofrog commented 10 years ago

Note to self - we need to add a volumes property to VoronoiGrid

astrofrog commented 10 years ago

@bluescarni - I've been testing this out extensively this week, and it's working great! I just made a number of small changes above that you can take a look at if you like :)

bluescarni commented 10 years ago

@astrofrog glad to hear it works :)

I went through the latest changes and they look good to me. I think we still need to figure out how to use the extra walls support (sphere, cylinder, etc.), but for now I guess it will do.

bluescarni commented 10 years ago

@astrofrog It looks like the tables now work ok, but a test fails because the hyperion_vor executable is not found. Indeed, I need to build it explicitly on my machine as it is not included in the default build configuration.

astrofrog commented 10 years ago

@bluescarni - ok, I tried to add that to see if it works!

bluescarni commented 10 years ago

@astrofrog cheers! was about to open another PR, you beat me to it.

bluescarni commented 10 years ago

@astrofrog tests are passing on 3 setups out of 5.

There is a strange out-of-memory failure on Python 3.4, which seems unrelated to the Voronoi part.

And there is a problem when deepcopying the neighbours table with Numpy 1.5 on Python 2.7. It complains that AttributeError: 'list' object has no attribute 'name'. I suppose we could avoid deepcopying (and return a reference to the internally-stored table instead), but it does not feel like a good solution.

astrofrog commented 10 years ago

@bluescarni - maybe we can just drop support for numpy 1.5, most installations now come with something more recent.

astrofrog commented 10 years ago

Is the deepcopying of tables something you can reproduce outside of Hyperion? If so, can you report it on the astropy issue tracker?

bluescarni commented 10 years ago

@astrofrog I am going to check about the deepcopying of the tables, I need to install locally a Numpy 1.5 first. If that turns out to be the problem we can get it fixed upstream then.

astrofrog commented 10 years ago

@bluescarni - ok, sounds good!

astrofrog commented 10 years ago

@bluescarni - it seems the table issue is the only one that remains after I restarted the jobs (the memory issue was intermittent)

astrofrog commented 10 years ago

@bluescarni - I removed some duplicate code in 941ad11e0a72ea59948c452eeee8655820dd9bde (this from what I had originally added) but other than that, this looks good to go!

astrofrog commented 10 years ago

All right, let's merge!

astrofrog commented 10 years ago

Thanks @bluescarni!