API update for OpenDX and MRC by spyke7 · Pull Request #164 · MDAnalysis/GridDataFormats

spyke7 · 2026-03-11T20:17:34Z

This is regarding #161

Implemented from_grid() a static method and native (property) inside OpenDX.py and mrc.py

Just simply added those two functions inside files
register converters
change export
write tests

codecov · 2026-03-11T20:19:03Z

Codecov Report

❌ Patch coverage is 85.30612% with 36 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.67%. Comparing base (b29c1f4) to head (868d607).

Files with missing lines	Patch %	Lines
gridData/OpenDX.py	81.81%	20 Missing and 10 partials ⚠️
gridData/core.py	57.14%	3 Missing ⚠️
gridData/mrc.py	95.89%	0 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #164      +/-   ##
==========================================
+ Coverage   88.20%   88.67%   +0.47%     
==========================================
  Files           5        5              
  Lines         814      839      +25     
  Branches      107      108       +1     
==========================================
+ Hits          718      744      +26     
- Misses         56       57       +1     
+ Partials       40       38       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

spyke7 · 2026-03-16T11:08:26Z

@orbeckst pls check this kindly

orbeckst

This is already looking pretty good. Please also add doc strings and update CHANGELOG. Will have a closer look once this is done. Thank you!

BradyAJohnston · 2026-03-19T18:39:53Z

gridData/tests/test_dx.py

+
+    assert isinstance(dx_field, gridData.OpenDX.field)
+
+    assert any('test_density' and 'test_user' in str(c) for c in dx_field.comments)


This is a logic error. 'test_density' and 'test_user' in str(c) evaluates as 'test_user' in str(c) due to Python's and short-circuit semantics — 'test_density' is truthy (not None / empty string), so it's ignored. Should be:

assert any('test_density' in str(c) for c in dx_field.comments) assert any('test_user' in str(c) for c in dx_field.comments)

Actually to get what you are doing there, you might need

assert any(('test_density' in str(c) and 'test_user' in str(c)) for c in dx_field.comments)

If you need them both to be true for a single comment.

assert any(('test_density' in str(c) and 'test_user' in str(c)) for c in dx_field.comments)

This is giving error -

> assert any(('test_density' in str(c) and 'test_user' in str(c)) for c in dx_field.comments) E assert False E + where False = any(<generator object test_dx_from_grid.<locals>.<genexpr> at 0x0000021CD04CF220>) gridData\tests\test_dx.py:104: AssertionError

Either I can do them separately or I can do this -
assert any(('test_density' and 'test_user') in str(c) for c in dx_field.comments)

BradyAJohnston · 2026-03-19T18:43:06Z

gridData/core.py

+
+    converter = {
+        'MRC': mrc.MRC.from_grid, 
+        'VDB': None, 


VDB is going to be implemented but it should be removed for now. Currently the self.converter[]() has potential to try and call None() so best to just not include until the VDB support is added (or add a guard for such in the later usage).

You can comment out this line.

BradyAJohnston · 2026-03-19T18:43:46Z

gridData/core.py

        grid, edges = g.histogramdd()
        self._load(grid=grid, edges=edges, metadata=self.metadata)

+    def convert_to(self, format_specifier, tolerance=None, **kwargs):


The tolerance is currently not used at all. Should be removed and left for the PR that introduces VDB.

BradyAJohnston · 2026-03-19T19:04:29Z

gridData/mrc.py

        if filename is not None:
            self.read(filename, assume_volumetric=assume_volumetric)

+    @staticmethod


Should these be classmethods instead of staticmethods?

In the issue it was staticmethod and it's fine for basic usage as well. But classmethod is more good, if somebody subclasses mrc/dx
Should I change it?

I would have thought it makes more sense to make it classmethod, but @orbeckst was specific about it being static method in the issue. @orbeckst I'm interested why?

I did't know any better ;-). After some reading I also recommend going with @classmethod — if we need to work with inheritance or use it from another classmethod then classmethod will work better.

orbeckst

Overall looking good but there are some necessary changes. In particular, the MRC.native should return a MrcFile.

Could you please also add a section to the gridData.core docs about using from_grid() and .native? Have a look at the issue text for #161 and you can probably copy and paste from there. We just want to document somewhere for users that this API exists.

orbeckst · 2026-03-19T20:59:41Z

CHANGELOG

 * 1.1.1

- Fixes
+  Changes


Enhancements (not changes)

orbeckst · 2026-03-19T22:15:01Z

CHANGELOG

-??/??/???? orbeckst
+??/??/???? orbeckst, spyke7

 * 1.1.1


Change to 1.2.0 (under semantic versioning, we must increase minor version for new features)

gridData/OpenDX.py

orbeckst · 2026-03-19T22:16:03Z

gridData/OpenDX.py

+        grid : Grid
+        type : str, optional
+        typequote : str, optional


add minimal explanations (can copy from other docs)

orbeckst · 2026-03-19T22:19:21Z

gridData/OpenDX.py

+
+    @property
+    def native(self):
+        """Return native object"""


Add a sentence "The "native" object is the :class:gridData.OpenDX.field itself."

add a .. versionadded:: 1.2.0 (with two blank lines above)

gridData/tests/test_dx.py

orbeckst · 2026-03-19T22:53:31Z

gridData/tests/test_dx.py

+    assert_equal(dx_field.components['data'].array, data)
+    assert_equal(dx_field.components['positions'].origin, g.origin)


These may be floats so use assert_allclose.

orbeckst · 2026-03-19T22:55:33Z

gridData/tests/test_dx.py

+    g = Grid(data, origin=[0, 0, 0], delta=[1, 1, 1])
+
+    dx_field = gridData.OpenDX.field.from_grid(g)
+    assert dx_field.native is dx_field


This checks that it's the same object. Add another assertion that checks the type explicitly

assert isinstance(dx_field.native, OpenDX.field)

orbeckst · 2026-03-19T22:56:37Z

gridData/tests/test_mrc.py

+
+        mrc_obj = mrc.MRC.from_grid(g)
+
+        assert mrc_obj.native is mrc_obj


Needs to change as the native object should be the mrcfile.MrcFile.

check type

check some values

orbeckst · 2026-03-19T22:57:48Z

gridData/tests/test_mrc.py

+
+        assert isinstance(mrc_obj, mrc.MRC)
+
+        assert_equal(mrc_obj.array, data)


Given that the data are integers, assert_equal is technically correct but for consistency and robustness, also use assert_allclose, please.

spyke7 · 2026-03-26T19:02:43Z

@orbeckst @BradyAJohnston what to do exactly for native for mrc.py, as I am not able to understand how to return a mrcfie.MrcFile?
Please review other changes, but kept that part same as previous.

orbeckst · 2026-03-26T21:00:34Z

Good point, MrcFile is tied to a file on disk. We could simply link it to a tmp file or maybe it works with StringIO? Or pass a filename in convert_to() and as optional kwarg? I have to think about this a little bit more and suggestions are welcome!

spyke7 · 2026-04-02T17:08:45Z

Good point, MrcFile is tied to a file on disk. We could simply link it to a tmp file or maybe it works with StringIO?

inside write() the filename can be a BytesIO object, as

if filename is not None:
            self.filename = filename

So ig it will work

Will also try the tempfile as well.

spyke7 · 2026-04-02T17:44:16Z

I used the tempfile at last cause of an error while using BytesIO

 def _open_file(self, name):
        """Open a file object to use as the I/O stream."""
>       self._iostream = open(name, self._mode + 'b')

mrcfile uses the open() function and it requires string not bytesIO

orbeckst · 2026-04-04T00:57:34Z

@spyke7 thanks for doing the experiments. Could we use mrcfile.mrcinterpreter.MrcInterpreter (instead of MrcFile) as it is built to take a stream?

orbeckst

Good that you got it to work! I would like to try and get MRC to work with a stream and avoid a temp file that is not cleaned up. Please see comments.

Tests need a bit more work and the coverage is also below threshold. Have a look at the coverage report to see which lines are not tested.

Good work!

CHANGELOG

orbeckst · 2026-04-04T01:27:54Z

gridData/mrc.py

+        fd, temp_path = tempfile.mkstemp(suffix=".mrc")
+
+        os.close(fd)
+        self.write(temp_path)
+
+        return mrcfile.open(temp_path)


This is a reasonable workaround but it's a bit clunky, we have to parse the data twice (once in write, then after open) and we leave a tempfile around that will fill up the disk.

A slightly better solution would be to self.write() to a buffer and then have mrcfile.mrcinterpreter.MrcInterpreter directly read the buffer. That at least avoids the file on disk issue.

If that works then perhaps we can move almost all code from write() into native and the write() would just write the internal stream buffer to a file and in this way we only parse the data once with mrcfile.

Let's try at least the "slightly better" solution — if that works then we can use it for now.

Thanks for the suggestions,
What I am doing is that inside of write I will check whether a stream is given or not and then inside it only, call the MrcInterpreter

orbeckst · 2026-04-04T01:32:29Z

gridData/tests/test_mrc.py

+        native_mrc = mrc_wrapper.native
+
+        assert isinstance(native_mrc, mrcfile.mrcfile.MrcFile)
+        np.testing.assert_allclose(native_mrc.data, data.T)


Why do we need to transpose data to make it agree?

If I do the native test with np.ones and symmetric matrix, then it will be no problem whether I give data or data.T
In case of (3,4,5) if I don't take the transpose it is throwing an AssertionError
as (x,y,z) -> (z,y,x)

the error -

AssertionError: E Not equal to tolerance rtol=1e-07, atol=0 E E (shapes (5, 4, 3), (3, 4, 5) mismatch)

dx is working fine with just checking with data, with non-symmetric and varying delta

https://mrcfile.readthedocs.io/en/stable/source/mrcfile.html#mrcfile.new_mmap

Check for shape

orbeckst · 2026-04-04T01:33:05Z

gridData/tests/test_mrc.py

+        assert mrc_obj.rank == 3
+
+    def test_mrc_native_property(self):
+        data = np.ones((3, 3, 3))


use a non-symmetric grid eg arange(60).reshape(3,4,5) ; with only ones you can miss issues with axes orientations.

orbeckst · 2026-04-04T01:34:15Z

gridData/tests/test_mrc.py

+
+    def test_mrc_native_property(self):
+        data = np.ones((3, 3, 3))
+        g = Grid(data, origin=[0, 0, 0], delta=[1, 1, 1])


Varydelta = [1., 3., 0.5]. Generally you want as little symmetry as possible so that you can catch errors that just transpose indices.

orbeckst · 2026-04-04T01:35:32Z

gridData/tests/test_dx.py

        err_msg="deltas of written grid do not match original")
+
+def test_dx_from_grid():
+    data = np.ones((5, 5, 5), dtype=np.float32)


use a non-symmetric grid eg arange(60).reshape(3,4,5)

orbeckst · 2026-04-04T01:35:43Z

gridData/tests/test_dx.py

+    assert_allclose(dx_field.components['positions'].origin, g.origin)
+
+def test_dx_native():
+    data = np.ones((5, 5, 5))


use a non-symmetric grid eg arange(60).reshape(3,4,5)

orbeckst · 2026-04-04T01:35:49Z

gridData/tests/test_dx.py

+
+def test_dx_native():
+    data = np.ones((5, 5, 5))
+    g = Grid(data, origin=[0, 0, 0], delta=[1, 1, 1])


orbeckst · 2026-04-04T01:36:08Z

gridData/tests/test_mrc.py

        assert_allclose(grid.grid, ccp4data.array)

+    def test_mrc_from_grid(self):
+        data = np.arange(27, dtype=np.float32).reshape((3, 3, 3))


even better: use a non-symmetric grid eg arange(60).reshape(3,4,5)

orbeckst · 2026-04-04T01:36:28Z

gridData/tests/test_mrc.py


+    def test_mrc_from_grid(self):
+        data = np.arange(27, dtype=np.float32).reshape((3, 3, 3))
+        g = Grid(data, origin=[1.0, 2.0, 3.0], delta=[0.5, 0.5, 0.5])


vary delta, e.g. [0.5, 1.5, 1.0]

orbeckst · 2026-04-04T01:42:01Z

Regarding coverage: Just check that the lines that you wrote get covered. You ran black on the whole file so this picks up any of those lines. You are not responsible for writing other tests if this was just a formatting change (this is one of the reasons why we normally do reformatting in separate PRs.)

orbeckst · 2026-04-04T01:46:34Z

Looking at https://app.codecov.io/gh/MDAnalysis/GridDataFormats/pull/164 it seems that the Grid.convert_to() method is not tested. This one is important :-) so make sure there are explicit tests for OpenDX and MRC.

Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>

spyke7 · 2026-04-04T19:04:52Z

So, my plan is this -

check whether the passed filename is io.BytesIO or not using isinstance
create a header with numpy using the header data type
that header will be given values - nx, ny, nz, mode, map, machst and then the inside the filename(i.e the stream) the header and the data_for_file will be written
else all other things same for write

mrcfile utils

problem is -
from a mrcfile.mrcinterpreter.MrcInterpreter object we need to convert it to mrcfile.mrcfile.Mrcfile
for that i guess, we need to tweak values inside __init__ of mrcfile.mrcfile.Mrcfile. That way we can convert it.

There is no other way of writing a stream except the interpreter. It would be helpful if I get some info regarding the Mrcfile codebase @orbeckst @BradyAJohnston pls see once when you have got time
Thanks

- MRC.native generates a new mrcfile.mrcinterpreter.MrcInterpreter instance - use the MrcInterpreter instance for writing out Note: only uncompressed files currently supported.

orbeckst · 2026-04-07T21:30:00Z

I updated with how I think this could work (git pull the changes). The tests passed locally. Please have a quick look and let me know what you think.

written from_grid and native for dx and mrc

fe00479

updated core.py, test_dx.py and test_mrc.py

4243676

orbeckst requested changes Mar 16, 2026

View reviewed changes

spyke7 changed the title ~~API update from OpenDX and MRC~~ API update for OpenDX and MRC Mar 19, 2026

spyke7 added 2 commits March 19, 2026 20:32

updated CHANGELOG and docstrings

7ec354b

reformatted with black

304abaf

spyke7 requested a review from orbeckst March 19, 2026 15:09

BradyAJohnston requested changes Mar 19, 2026

View reviewed changes

orbeckst requested changes Mar 19, 2026

View reviewed changes

fixed CHANGELOG, added docstrings and updated tests

682b6ab

Used tempfile to store the mrcfile inside native

b570316

imported mrcfile inside test_mrc.py

df60e81

spyke7 requested a review from orbeckst April 2, 2026 18:00

orbeckst requested changes Apr 4, 2026

View reviewed changes

Update CHANGELOG

7bab5b2

Co-authored-by: Oliver Beckstein <orbeckst@gmail.com>

use MrcInterpreter as native MRC object

868d607

- MRC.native generates a new mrcfile.mrcinterpreter.MrcInterpreter instance - use the MrcInterpreter instance for writing out Note: only uncompressed files currently supported.


		assert isinstance(dx_field, gridData.OpenDX.field)

		assert any('test_density' and 'test_user' in str(c) for c in dx_field.comments)

                * 1.1.1
-               Fixes
+                Changes

		assert_equal(dx_field.components['data'].array, data)
		assert_equal(dx_field.components['positions'].origin, g.origin)


		mrc_obj = mrc.MRC.from_grid(g)

		assert mrc_obj.native is mrc_obj


		assert isinstance(mrc_obj, mrc.MRC)

		assert_equal(mrc_obj.array, data)

Conversation

spyke7 commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

spyke7 commented Mar 16, 2026

Uh oh!

orbeckst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spyke7 Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BradyAJohnston Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orbeckst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spyke7 commented Mar 26, 2026

Uh oh!

orbeckst commented Mar 26, 2026

Uh oh!

spyke7 commented Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spyke7 commented Apr 2, 2026

Uh oh!

orbeckst commented Apr 4, 2026

Uh oh!

orbeckst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

spyke7 commented Mar 11, 2026 •

edited

Loading

codecov bot commented Mar 11, 2026 •

edited

Loading

spyke7 Mar 19, 2026 •

edited

Loading

BradyAJohnston Mar 19, 2026 •

edited

Loading

spyke7 commented Apr 2, 2026 •

edited

Loading