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 27 commits intoMDAnalysis:masterfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #148 +/- ##
==========================================
+ Coverage 88.20% 88.82% +0.62%
==========================================
Files 5 6 +1
Lines 814 913 +99
Branches 107 124 +17
==========================================
+ Hits 718 811 +93
- Misses 56 60 +4
- Partials 40 42 +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
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 |
|
Are you looking for a workflow such as the following @PardhavMaradani g = gdf.Grid("density.dx")
# make our VDB-like object that contains .vdb_grid as the VDB grid (eg FloatGrid)
gdf_vdb = gdf.OpenVDB.field(g)
# Then work with the VDB instance `vdb_grid`
gdf_vdb.vdb_grid.transform = createLinearTransform(matrix)
...If you provide code examples for how you would like to be able to use gdf then this would make things clear. |
Hi @orbeckst , yes, something along those lines. Here are some examples: Regular export from GDF: g = gdf.Grid("density.dx")
g.export("density.vdb", ...)The export options for above are any minimally required ones for basic functionality. Regular import into GDF: g = gdf.Grid("density.vdb")For someone like MN or others who want to add additional transforms and metadata to the vdb grid: g = gdf.Grid("density.dx")
vdb_grid = gdf.OpenVDB.grid_to_vdb(g)
vdb_grid.transform.preScale(...)
vdb_grid.transform.postTranslate(...)
vdb_grid["metadata_key_1"] = supported_type_value1
gdf.OpenVDB.write(vdb_grid, "/tmp/custom_grid.vdb", ...)
import openvdb
g1 = gdf.Grid("density1.dx")
g2 = gdf.Grid("density2.ccp4")
vdb_grid1 = gdf.OpenVDB.grid_to_vdb(g1)
vdb_grid2 = gdf.OpenVDB.grid_to_vdb(g2)
openvdb.write("/tmp/multiple_grids.vdb", grids=[vdb_grid1, vdb_grid2])The last two examples show how access to the openvdb grid can help with extensibility. Based on the above use cases,
The exporter could look something like: def _export_vdb(self, filename, ...):
...
gdf.OpenVDB.write(self, filename, ...)The importer could look like: def _load_vdb(self, filename, ...):
...
g = gdf.OpenVDB.read(filename, ...)
self._load(grid=g.grid, edges=g.edges, ...)Others who require additional |
gridData/OpenVDB.py
Outdated
| from gridData import OpenVDB | ||
| vdb_field = OpenVDB.field('density') | ||
| vdb_field.populate(grid, origin, delta) | ||
| vdb_field.write('output.vdb') |
gridData/OpenVDB.py
Outdated
| vdb_field = OpenVDB.field('density') | ||
| vdb_field.populate(grid, origin, delta) | ||
| vdb_field.write('output.vdb') | ||
|
|
|
Thanks for the use cases @PardhavMaradani, that's very helpful to see. We might be able to have gdf.OpenVDB contain simple functions and then introduce "convertors" for API interoperability (similar to what MDAnalysis is offering in the converter module. For instance, g = gdf.Grid("density.dx") # -> gdf.Grid
v = g.convert_to("vdb") # -> openvdb.GridBaseOnce we have this functionality, export is just doing this conversion before calling We can then also consider extending the converters to MRC objects. Eventually we could also add the functionality to drop OpenVDB or MRC objects into If we do a converter-style API then the gdf.OpenVDB module can be pretty light-weight because we don't really expect users to directly work with it. Does this sound like an interesting/clean way forward? |
|
@spyke7 I wanted to say that you're doing good work here! Don't be discouraged by the long discussions and the possibility that we want to change things again. You've demonstrated that the core of your code is working, now we can think about how this will best work long-term. Creating code that is actually used by people requires thought and discussion. The fact that we're having these discussions over your code means that this is something that we believe will have a long-term impact and is important enough to get right. |
yeah ofcourse, I will try the best updating the changes. And I can keep track of messages and reviews! Thanks. |
Using a generic |
Bringing this up from a review comment above to see if there is any possible way to address this as it seems a bit limiting.
|
It's a bit annoying when you include code that you cannot fully test under "normal" (i.e. conda-forge packages circumstances), although we would still write tests with mock-functionality. What would the logic be for the correspondence between numpy dtypes and OpenVDB grid types? Does the following look sensible (produced with ChatGTP 5.2) where the 3rd column is the fall back default when we only have the limited build of OpenVDB:
(EDIT: updated |
|
The above list looks good. The only minor change would be:
One reference for the Grid types that OpenVDB will register by default can be found here (search for |
|
I created #160 as parent issue for the converter (API-interoperability) approach. I think we can make some changes here that will set us up to do the full API-interoperability in separate PRs. |
|
We should address the data type selection in this PR, though. |
orbeckst
left a comment
orbeckst
left a comment
There was a problem hiding this comment.
We'd like to make the code ready for API interoperability (see #160) so the major change is to just create the OpenVDB grid in init and store it as an attribute.
We also want to be able to choose the best OpenVDB grid datatype. There are different ways in which this can be done and I suggested one. Testing will be a bit tricky and will require mocking together with a fair number of combinations but I think this can be done with parametrized tests and doesn't have to be hard coded.
If you have questions please ask.
| self.grid = None | ||
| self.origin = None | ||
| self.delta = None | ||
|
|
There was a problem hiding this comment.
Create the OpenVDB data structure in __init__ and keep it as an attribute vdb_grid.
gridData/OpenVDB.py
Outdated
| if self.grid.dtype == numpy.bool_ or self.grid.dtype == bool: | ||
| vdb_grid = vdb.BoolGrid() | ||
| use_tolerance = False | ||
|
|
||
| else: | ||
| vdb_grid = vdb.FloatGrid() | ||
| if self.tolerance == None or self.tolerance == 0: | ||
| use_tolerance = False | ||
| else: | ||
| use_tolerance = True | ||
|
|
||
| vdb_grid.name = self.name | ||
|
|
||
| vdb_grid.transform = vdb.createLinearTransform() | ||
| vdb_grid.transform.preScale(self.delta.tolist()) | ||
| vdb_grid.transform.postTranslate(self.origin.tolist()) | ||
|
|
||
| if self.metadata: | ||
| for key, val in self.metadata.items(): | ||
| try: | ||
| vdb_grid[key] = val | ||
| except (TypeError, ValueError) as e: | ||
| warnings.warn(f"Could not set metadata '{key}': {e}", UserWarning) | ||
|
|
||
| if use_tolerance: | ||
| vdb_grid.copyFromArray(self.grid, tolerance=self.tolerance) | ||
| vdb_grid.prune() | ||
| else: | ||
| vdb_grid.copyFromArray(self.grid) | ||
| vdb_grid.prune(tolerance=False) |
There was a problem hiding this comment.
Move all of the code for creating the OpenVDB grid object in a separate private method (e.g., _create_openvdb_grid(self, ...)) and use it in __init__.
Then have write() just access the openvdb grid and shorten the code here to 1-2 lines.
gridData/OpenVDB.py
Outdated
| if self.grid.dtype == numpy.bool_ or self.grid.dtype == bool: | ||
| vdb_grid = vdb.BoolGrid() | ||
| use_tolerance = False | ||
|
|
||
| else: | ||
| vdb_grid = vdb.FloatGrid() | ||
| if self.tolerance == None or self.tolerance == 0: | ||
| use_tolerance = False | ||
| else: | ||
| use_tolerance = True |
There was a problem hiding this comment.
Refine these code to choose the best available OpenVDB data type, based on the table in #148
I would probably create a dict such as
datatypes = {np.bool: ["BoolGrid"],
np.int8: ["Int32Grid", "FloatGrid"],
np.uint8: ["Int32Grid", "FloatGrid"],
...
np.float16: ["HalfGrid", "FloatGrid"],
np.float32: ["FloatGrid"],
np.float64: ["DoubleGrid", "FloatGrid"],
...}
data = grid.array
try:
vdb_gridtypes = datatypes[data.dtype]
except KeyError:
raise TypeError(f"data type {data.dtype} is not supported for VDB")
# look for the best matching grid type by searching the list of
# gridtypes in order
for gridtype in vdb_gridtypes:
try:
VDB_Grid = getattr(openvdb, gridtype)
except AttributeError:
# not compiled with appropriate support
# try next one
pass
else:
break
else:
raise TypeError(f"Could not find VDB Grid {gridtype} for numpy type {data.dtype}")
# create the VDB grid instance
vdb = VDB_Grid(....)
...What's missing here is the ability to issue warnings when we are down-converting. Perhaps the easiest way to do this is to create another list with all the GridTypes that need a warning, or make the datatypes dict more complicated with entries such as np.dtype: (["PreferredGrid", "DefaultGrid"], warn: bool).
There was a problem hiding this comment.
One other potential alternative to specify whether it is a downcast (and hence needs a warning) is to specify it in the same list with a simple dataclass wrapper.
For example:
from dataclasses import dataclass
@dataclass
class DownCastTo:
gridType: str
datatypes = {
...,
np.float64: ["DoubleGrid", DownCastTo("FloatGrid")],
...,
}An instanceof check could be used later to both determine the downcast and get the type as follows:
...
if isinstance(gridtype, DownCastTo):
gridtype = gridtype.gridType
...This will keep it all in a single list and also allow this to be per-type. I don't however know if this is a good/bad idea and an appropriate use of the dataclass. Thanks
There was a problem hiding this comment.
@PardhavMaradani should I do the center and scale ?
The reason we are adding convert_to is to allow users to access the vdb grid directly and transform it as they please. So as long as this and the ability to write the modified vdb grid exists, we don't need any explicit transforms like the above within GDF. MN and potentially others will follow that path. Thanks
|
Here is the screenshots and process that I have followed to show that, the
(.vdb)
|
orbeckst
left a comment
orbeckst
left a comment
There was a problem hiding this comment.
Overall this looks pretty good! Please see comments inline.
gridData/OpenVDB.py
Outdated
| """ | ||
| datatypes = { | ||
| numpy.dtype("bool"): ["BoolGrid"], | ||
| numpy.dtype("int8"): [DownCastTo("Int32Grid"), DownCastTo("FloatGrid")], |
There was a problem hiding this comment.
https://en.wikipedia.org/wiki/IEEE_754#cite_ref-16 says that Single (float) has 23 bits in the mantissa which is enough to represent all int8 exactly.
(note: int8 covers -127 to 127!)
gridData/OpenVDB.py
Outdated
| datatypes = { | ||
| numpy.dtype("bool"): ["BoolGrid"], | ||
| numpy.dtype("int8"): [DownCastTo("Int32Grid"), DownCastTo("FloatGrid")], | ||
| numpy.dtype("uint8"): [DownCastTo("Int32Grid"), DownCastTo("FloatGrid")], |
gridData/OpenVDB.py
Outdated
| numpy.dtype("bool"): ["BoolGrid"], | ||
| numpy.dtype("int8"): [DownCastTo("Int32Grid"), DownCastTo("FloatGrid")], | ||
| numpy.dtype("uint8"): [DownCastTo("Int32Grid"), DownCastTo("FloatGrid")], | ||
| numpy.dtype("int16"): [DownCastTo("Int32Grid"), DownCastTo("FloatGrid")], |
gridData/OpenVDB.py
Outdated
| numpy.dtype("int8"): [DownCastTo("Int32Grid"), DownCastTo("FloatGrid")], | ||
| numpy.dtype("uint8"): [DownCastTo("Int32Grid"), DownCastTo("FloatGrid")], | ||
| numpy.dtype("int16"): [DownCastTo("Int32Grid"), DownCastTo("FloatGrid")], | ||
| numpy.dtype("uint16"): [DownCastTo("Int32Grid"), DownCastTo("FloatGrid")], |
gridData/OpenVDB.py
Outdated
| numpy.dtype("uint32"): ["Int32Grid", DownCastTo("FloatGrid")], | ||
| numpy.dtype("int64"): ["Int64Grid", DownCastTo("FloatGrid")], | ||
| numpy.dtype("uint64"): ["Int64Grid", DownCastTo("FloatGrid")], | ||
| numpy.dtype("float16"): [DownCastTo("HalfGrid"), DownCastTo("FloatGrid")], |
gridData/OpenVDB.py
Outdated
| for gridtype_downcast in vdb_gridtypes: | ||
| if isinstance(gridtype_downcast, DownCastTo): | ||
| gridtype = gridtype_downcast.gridType | ||
| else: | ||
| gridtype = gridtype_downcast |
There was a problem hiding this comment.
This looks a bit ugly but I also don't have a great idea at the moment.
gridData/OpenVDB.py
Outdated
| if isinstance(gridtype_downcast, DownCastTo): | ||
| warnings.warn( | ||
| f"Grid type {vdb_gridtypes[0]} not available. Using {gridtype} instead. Data may lose precision.", | ||
| UserWarning, |
There was a problem hiding this comment.
Make this a RuntimeWarning.
(I think that better reflects that it's really due to the installed openvdb package not having the full support.)
gridData/OpenVDB.py
Outdated
| ) | ||
| return VDB_Grid() | ||
|
|
||
| raise TypeError( |
There was a problem hiding this comment.
make it an else: for the for loop to make the control flow clearer.
gridData/OpenVDB.py
Outdated
| f"Grid type {vdb_gridtypes[0]} not available. Using {gridtype} instead. Data may lose precision.", | ||
| UserWarning, | ||
| ) | ||
| return VDB_Grid() |
There was a problem hiding this comment.
Put the return at the end of method. What you're doing here isn't wrong but I think it's clearer when we return at the end with the desired result instead somewhere in the middle. The control structures all support it (using else!) and it makes it possible to continue processing outside the loop if necessary.
There was a problem hiding this comment.
pls check it now,
This looks a bit ugly but I also don't have a great idea at the moment.
and for the over all flow, any other ideas. Like I simply came up with that, but it can be made short and complex..
|
I will be pretty busy for the next 1-2 weeks and may not be able to spend much time on in-depth reviewing. |
Sure! No problem with that, in the mean time I can implement any other features and changes if told by @BradyAJohnston and @PardhavMaradani or can try to make the |
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.