added a simple program to export files in .vdb format#148
added a simple program to export files in .vdb format#148spyke7 wants to merge 38 commits intoMDAnalysis:masterfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #148 +/- ##
==========================================
+ Coverage 89.69% 89.89% +0.20%
==========================================
Files 5 6 +1
Lines 844 950 +106
Branches 108 125 +17
==========================================
+ Hits 757 854 +97
- Misses 52 59 +7
- Partials 35 37 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@orbeckst , please review the OpenVDB.py file. After that, I will add some more test covering all the missing parts |
|
Thank you for contribution. I’m currently on holidays and will come back to reviewing open source contributions in the new year. Am 12/27/25 um 03:16 schrieb Shreejan Dolai ***@***.***>:spyke7 left a comment (MDAnalysis/GridDataFormats#148)
@orbeckst , please review the OpenVDB.py file. After that, I will add some more test covering all the missing parts
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
orbeckst
left a comment
There was a problem hiding this comment.
Thank you for your contribution. Before going further, can you please try your own code and demonstrate that it works? For instance, take some of the bundled test files such as 1jzv.ccp4 or nAChR_M2_water.plt, write it to OpenVDB, load it in blender, and show an image of the rendered density?
Once we know that it's working in principle, we'll need proper tests (you can look at PR #147 for good example of minimal testing for writing functionality).
| Fixes | ||
|
|
||
| * Adding openVDB formats (Issue #141) |
There was a problem hiding this comment.
not a fix but an Enhancement – put it into the existing 1.1.0 section and add you name there.
There was a problem hiding this comment.
In the CHANGELOG, this PR and issue are in the 1.1.0 release, so should I add my name in the 1.1.0 release or remove those lines and put them in the new section?
There was a problem hiding this comment.
Yes, now move it to the new section above since we released 1.1.0.
| for i in range(self.grid.shape[0]): | ||
| for j in range(self.grid.shape[1]): | ||
| for k in range(self.grid.shape[2]): | ||
| value = float(self.grid[i, j, k]) | ||
| if abs(value) > threshold: | ||
| accessor.setValueOn((i, j, k), value) |
There was a problem hiding this comment.
This looks really slow — iterating over a grid explicitly. For a start, you can find all cells above a threshold with numpy operations (np.abs(g) > threshold) and then ideally use it in a vectorized form to set the accessor.
|
fixed the CHANGELOG and OpenVDB.py. I didn't get the time to work on the blender part due to exams. I will surely try do it! |
|
Good that you're able to load something into Blender. From a first glance I don;t recognize what I'd expect but this may be dependent on how you render in Blender. As I already said on Discord: Try to establish yourself what "correct" means. Load the original data in a program where you can reliably look at it. ChimeraX is probably the best for looking at densities; it can definitely read DX. Btw, the M2 density should look similar to the blue "blobs" on the cover of https://sbcb.bioch.ox.ac.uk/users/oliver/download/Thesis/OB_thesis_2sided.pdf |
|
Mentioned in the Discord but also bringing up here: In your current examples (most obvious with the pore) is that the axis is flipped so that X is "up" compared to atomic coordinates which would have Z as up. |
Thank you for the update! will try to fix this |
|
Ideally we would see this alongside the atoms or density from MN as well - to double check alignment because you might also need to flip one of the X or Y axes. |
|
The scales might be different (larger or smaller by factors of 10) but you can just scale inside of Blender by that amount to align the scales, but we want to be double checking alignemnt and axes. |
|
I have first of all added the MolecularNode add-on as given in the https://github.com/BradyAJohnston/MolecularNodes, and imported the 1jzv.pdb. After that import the .vdb file and there was difference in size of two. So I made the size the .pdb bigger. The centers of both of them are same and I didn't flipped any of the axes in the ss provided. I wrote a small blender py script to compare bounding boxes of the pdb and vdb objects to verify centroids, extents and axis alignment- output - The centroids are almost same I guess... |
|
@spyke7 It's still not 100% clear from your screenshots - can you import with the pore instead as that is more clear? And when you are taking a screenshot it would be more helpful to have the imported density in the centre of the screen rather than mostly empty space. |
|
Looks like you are attempting a standalone export to |
|
If this functionality can be added directly to GDF then we can also take advantage of that in MN going forwards. |
Agreed. In addition to exporting to |
This is a good point and something to consider as well. As far as I am aware Blender / MN (and other 3D animation packages) might be the only ones who use If there is anything out there that does take |
orbeckst
left a comment
There was a problem hiding this comment.
The PR is in a good shape. However, my understanding is that we want this to work with a convert_to(). Thus, we actually need #161 first and then come back here.
I know this is getting long @spyke7 . As I said before, if you didn't do interesting work, we wouldn't have this conversation about major changes to the package.
Are you able to take on #161 ?
Sorry, for not updating on #160 or #161 , as I was also busy for the past week (class internals and assignments 🥲). |
|
Need some guidance to write docstrings for is there a way to get the grid without loosing tolerance? like I'm getting the original grid from |
|
Removed the last few commits, so as to remove the new edits/updates for |
|
Ok, then close PR #163 please! |
orbeckst
left a comment
There was a problem hiding this comment.
This looks pretty good. I have a few comments that need addressing but for most of them you can probably just "accept suggestion".
There's some code that's not yet covered by tests. Please look at the coverage reports. For instance https://app.codecov.io/gh/MDAnalysis/GridDataFormats/pull/148?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=checks&utm_campaign=pr+comments&utm_term=MDAnalysis shows that OpenVDBFiled.from_grid() and .native are not tested but these are important for the API so we need explicit tests.
Also run black on the whole file. Hopefully it re-orders the imports so that standard lib imports come before external packages.
The core code looks all good so should be an easy merge.
| Enhancements | ||
|
|
||
| * Added openVDB format exports (Issue #141, PR #148) | ||
|
|
There was a problem hiding this comment.
Make sure that there's only a single Enhancements section by adding your new line below.
Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>
Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>
Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>
|
The latest change improved the coverage but it will still be important to test the new attributes and functions explicitly. |
|
Hi @orbeckst, sorry for late reply. This piece of test code works fine - from gridData import Grid
g = Grid(str(filename))
g.export(str(outputfile), tolerance=1e-2)But if I try to take out the wrapper and pass it to the same g = Grid(str(filename))
grid1 = g.convert_to("vdb") # -> native wrapper
grid2 = Grid(grid=grid1) # -> Erroras edges, origin and delta are None. Quick workaround is using the openvdb itself # Above code without grid2
import openvdb
openvdb.write(str(output_file), grids = [grid1])So I was thinking if inside from_grid, we can extract origin, delta, and edges from provided |
grid1 = g.convert_to("vdb") # -> native wrapper
grid2 = Grid(grid=grid1) # -> Error
Then the second line is really what #162 is about — that could wait until we address that issue. |
|
At this stage I'd be happy to merge this PR if you address my comments and include explicit tests for the The changes that I made test these implicitly so coverage is now sufficient but we should have explicit tests for key API functionality. @PardhavMaradani I know that you're busy. Let us know if you would like to have some extra time for reviewing or if you're ok with proceeding as necessary. |
Yeah, I was looking into other comments, and will create the tests as required. |










Hi @orbeckst
I have added
OpenVDB.pyinside gridData that simply export files in.vdbformat. Also I have addedtest_vdb.pyinside tests and it successfully passes.fix #141
Required Libraries -
openvdb
conda install -c conda-forge openvdbThere are many things that need to be updated like docs, etc, but I have just provided the file and test so that you can review it, and I can fix the problems. Please let me know if anything needs to be changed and updated.