added a simple program to export files in .vdb format #148
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #148 +/- ##
==========================================
+ Coverage 88.20% 88.64% +0.43%
==========================================
Files 5 6 +1
Lines 814 863 +49
Branches 107 115 +8
==========================================
+ Hits 718 765 +47
- Misses 56 57 +1
- Partials 40 41 +1 ☔ 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
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).
CHANGELOG
Outdated
| 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.
gridData/OpenVDB.py
Outdated
| 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! |
The first two are for naChR_M2_water.vdb and the last two are for 1jzv.vdb. |
|
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 |
The first one is for 1jzv.vdb and second for nAChR_M2_water.vdb (not done the shading/coloring) Can I proceed with the tests part? |
|
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 |
I think this fixes the axis.. |
|
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 |
|
Some thoughts:
|
Yes, when I import the .vdb as export by the OpenVDB.py it is different in scale than the one import by the MN-add on. Also, I need to add the geometry node as I showed in the video, and change the threshold which is ISO in this case, to make it look like same as that of MN-add on |
if I can make a video showing all the things about importing the |
|
If a video is necessary then show it. Please also write out (in text) the individual steps that you are carrying out in the video. (Videos just take a lot of time to watch and are often ambiguous. Text is much faster to digest and more succinct because the writer (hopefully) took the time to think about how to clearly convey the message.) |
|
@PardhavMaradani many thanks for your input:
Yes, I agree, the goal here is to have something that works generically and would be easy for MN to adopt in order to reduce code-duplication.
I agree. Does the current PR hard-code any of these transformations? Can you give an example of what this should look like?
I'd be ok to have this PR only export and raise a new issue for reading VDB files, which looks very doable, given that @spyke7 is already using some of this openvdb functionality in the tests (which is good!)
That's good motivation. Maybe @spyke7 would be interested to work on it after this PR? |
gridData/OpenVDB.py
Outdated
| Output filename (should end in .vdb) | ||
|
|
||
| """ | ||
| self.grid=numpy.ascontiguousarray(self.grid, dtype=numpy.float32) |
There was a problem hiding this comment.
If this need to be done, do it in __init__. It's confusing to change attributes as a side-effect of writing.
| ] | ||
|
|
||
| vdb_grid.background = 0.0 | ||
| vdb_grid.transform = vdb.createLinearTransform(matrix) |
There was a problem hiding this comment.
I assume that the transformation is required to make the VDB grid to have the correct origin and delta in general.
Or is this something specific to the MN blender use, @PardhavMaradani ?
There was a problem hiding this comment.
Or is this something specific to the MN blender use, @PardhavMaradani ?
I addressed this in the comment below. Thanks
There was a problem hiding this comment.
I saw the tests (eg test_write_vdb_with_delta_matrix) that checked that reading the VDB file would reproduce the original delta and my understanding is that this works because of the transformations added here. I think it's quite important that we can roundtrip consistently so I would leave the transformations as they are as a default. (Correct me if I am wrong, please.)
If MN/Blender needs to scale/shift then we should make this possible on top of the default.
gridData/core.py
Outdated
| """ | ||
| if self.grid.ndim != 3: | ||
| raise ValueError( | ||
| "OpenVDB export requires a 3D grid, got {}D".format(self.grid.ndim)) |
gridData/tests/test_vdb.py
Outdated
| def test_write_vdb_nonuniform_spacing_warning(self, tmpdir): | ||
| data = np.ones((3, 3, 3), dtype=np.float32) | ||
| delta = np.array([0.5, 1.0, 1.5]) | ||
| g = Grid(data, origin=[0, 0, 0], delta=delta) | ||
|
|
||
| outfile = str(tmpdir / "nonuniform.vdb") | ||
| g.export(outfile) | ||
| assert tmpdir.join("nonuniform.vdb").exists() |
There was a problem hiding this comment.
What are we testing here?
Why does the name contain "warning"?
There was a problem hiding this comment.
this only checks if the file exists with non-uniform delta
There was a problem hiding this comment.
I think in the end I can check whether each axis has correct spacing. I will update it
gridData/tests/test_vdb.py
Outdated
| @pytest.mark.skipif(HAS_OPENVDB, reason="Testing import error handling") | ||
| def test_vdb_import_error(): | ||
| with pytest.raises(ImportError, match="pyopenvdb is required"): | ||
| gridData.OpenVDB.OpenVDBField( | ||
| np.ones((3, 3, 3)), | ||
| origin=[0, 0, 0], | ||
| delta=[1, 1, 1] |
There was a problem hiding this comment.
You should be able to test the import handling when the test suite is being run. Look into mocking https://docs.python.org/3/library/unittest.mock.html (and there may also be something for pytest) – basically, make it so that just when this test function is run, the openvdb module is not imported.
There was a problem hiding this comment.
I have not done this in the recent push. Will be doing it!
There is a hard-coded linear transformation in the current code as you pointed out in a review comment above. It is unclear to me what it does - @spyke7 could you please help explain (maybe I'm reading this incorrectly, if it is a combined scale and translation matrix, shouldn't the translations be in the 4th column and not row?)
This exporter could have additional args (similar to def _export_vdb(
self,
filename,
center: bool = False,
scale: float = 1.0,
metadata: dict = None,
...
):
...The Ideally, any grid metadata should be exported as I am not sure if the |
Yes. This is a row major matrix (used in the code). The one you are saying is the column major matrix where the translation is in the 4th column. |
Sure! |
yes, threshold is used to set the tolerance. |
So for example if I use, and inside the function change, if this can be done, then we can pass scale, tolerance and center. Should I do it @orbeckst as it is present in the comments that it can process kwargs but not required to do so... |
|
Yes, sort of: you need to add additional explicit keyword arguments to the top-level |
|
See #149 (comment) for a discussion for why we want to have explicit keywords. |
|
@PardhavMaradani can you please explain the center variable, more? As I cannot understand what to do for this.. |
The Here is an example of a density file (apbs.dx.gz) that is centered (left) and not (right): 
Here is a front view of the above: 
Here is a snippet from the code I pointed out in a previous comment: if center:
offset = -np.array(grid.shape) * 0.5 * gobj.delta
else:
offset = np.array(gobj.origin)
# apply transformations
vdb_grid.transform.preScale(np.array(gobj.delta) * world_scale)
vdb_grid.transform.postTranslate(offset * world_scale)If |
|
Thinking about this a bit more - @BradyAJohnston , given that the centering and scaling are just world transforms, do we really need to impose this upon |
orbeckst
left a comment
orbeckst
left a comment
There was a problem hiding this comment.
Minor changes, please run black on the files to get all formatting consistent.
Regarding the transformations it actually looks reasonable to me, but I want to hear more from @BradyAJohnston and @PardhavMaradani .
gridData/tests/test_vdb.py
Outdated
| assert tmpdir.join("auto.vdb").exists() | ||
|
|
||
| def test_write_vdb_with_metadata(self, tmpdir): | ||
| data = np.ones((3, 3, 3), dtype=np.float32) |
There was a problem hiding this comment.
Could use grid345 and then add metadata.
gridData/tests/test_vdb.py
Outdated
|
|
||
| class TestVDBWrite: | ||
| def test_write_vdb_from_grid(self, tmpdir, grid345): | ||
| data,g = grid345 |
gridData/tests/test_vdb.py
Outdated
| got = acc.getValue((i, j, k)) | ||
| assert got == pytest.approx(float(data[i, j, k])) | ||
|
|
||
| def test_write_vdb_default_grid_name(self, tmpdir): |
gridData/tests/test_vdb.py
Outdated
|
|
||
| voxel_size = grid_vdb.transform.voxelSize() | ||
|
|
||
| spacing=[voxel_size[0], voxel_size[1], voxel_size[2]] |
gridData/tests/test_vdb.py
Outdated
| vdb_field.write(outfile) | ||
|
|
||
| grids, metadata = vdb.readAll(outfile) | ||
| assert grids[0].name == 'direct_test' |
gridData/tests/test_vdb.py
Outdated
|
|
||
| spacing = [voxel_size[0], voxel_size[1], voxel_size[2]] | ||
|
|
||
| assert_allclose(spacing, [1.0, 2.0, 3.0], rtol=1e-5) |
There was a problem hiding this comment.
instead of [1.0, 2.0, 3.0] use the variable
| assert_allclose(spacing, [1.0, 2.0, 3.0], rtol=1e-5) | |
| assert_allclose(spacing, delta, rtol=1e-5) |
gridData/tests/test_vdb.py
Outdated
| assert acc.getValue((2, 3, 4)) == pytest.approx(5.0) | ||
| assert acc.getValue((7, 8, 9)) == pytest.approx(10.0) |
There was a problem hiding this comment.
instead of hard coding 5.0 and 10.0, access data
| assert acc.getValue((2, 3, 4)) == pytest.approx(5.0) | |
| assert acc.getValue((7, 8, 9)) == pytest.approx(10.0) | |
| assert acc.getValue((2, 3, 4)) == pytest.approx(data[2, 3, 4]) | |
| assert acc.getValue((7, 8, 9)) == pytest.approx(data[7, 8, 9]) | |
(and one could just make it a loop over index tuples if there were more than 2)
gridData/tests/test_vdb.py
Outdated
| grid_vdb = grids[0] | ||
| acc = grid_vdb.getAccessor() | ||
|
|
||
| assert acc.getValue((1, 1, 1)) == pytest.approx(1.0) |
| ] | ||
|
|
||
| vdb_grid.background = 0.0 | ||
| vdb_grid.transform = vdb.createLinearTransform(matrix) |
There was a problem hiding this comment.
I saw the tests (eg test_write_vdb_with_delta_matrix) that checked that reading the VDB file would reproduce the original delta and my understanding is that this works because of the transformations added here. I think it's quite important that we can roundtrip consistently so I would leave the transformations as they are as a default. (Correct me if I am wrong, please.)
If MN/Blender needs to scale/shift then we should make this possible on top of the default.
|
In this recent push, I have just applied the changes as asked in test_vdb.py. Will soon implement the scale and center in |
|
Yeah!, I added that 0.5 * delta offset because GDF uses a cell-centered convention. I will remove this, as this is just creating additional offset. |
There was a problem hiding this comment.
This looks all really good to me.
From my perspective, we only need to decide if the transformations should stay.
EDIT: Only just saw #148 (comment) — so we're keeping the transformation but remove the offset.
@BradyAJohnston @PardhavMaradani want some way to tweak the exports. Could you please leave a (blocking) review describing what you need to have added so that MN can make best use of the functionality?
|
Sorry should have time to look over this tomorrow. Adding the offset / centering on export is definitely something that could be handled by MN, but adding some transformation to the grid on export might still be useful more generally (or adding a transform as a Grid before export?). Will look over in more detail tomorrow. |
4 participants
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.