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 81.53846% with 36 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.60%. Comparing base (b29c1f4) to head (304abaf).

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	86.95%	0 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #164      +/-   ##
==========================================
+ Coverage   88.20%   88.60%   +0.40%     
==========================================
  Files           5        5              
  Lines         814      834      +20     
  Branches      107      108       +1     
==========================================
+ Hits          718      739      +21     
- 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.

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)

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

gridData/OpenDX.py

+        Returns
+        -------
+        field
+            OpenDX field wrapper


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

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)

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

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)
+    # assert any('test_user' in str(c) for c in dx_field.comments)


remove commented out code

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.

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


		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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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