Better volumetric test #149

PlethoraChutney · 2026-01-13T23:04:01Z

Hello! Thank you for MDAnalysis! This PR changes how MRC files are checked before being loaded as a 3D grid. Rather than use the is_volume() method from mrcfile we instead use a simpler shape check.

The reason for this is that mrcfile returns False if the ISPG header field of a 3D MRC file is zero. While this is correct according to the spec, most cryoEM software packages save a 0 in this field for a variety of reasons. Because of this unwritten deviation from spec, most packages also read in files with a 0 space group as 3D Volumes anyway, but GridDataFormats does not.

Despite the fact that this is formally correct, I'm proposing that MDAnalysis should follow the behavior that users likely expect from their experience with other packages.

This PR fails three tests, but on my machine at least the current master branch also fails these same tests.

gridData/tests/test_grid.py::TestGrid::test_load_wrong_fileformat[ccp4]
gridData/tests/test_grid.py::TestGrid::test_load_wrong_fileformat[plt]
gridData/tests/test_grid.py::TestGrid::test_load_wrong_fileformat[dx]

codecov · 2026-01-13T23:05:23Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.76%. Comparing base (abd0feb) to head (a405729).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #149      +/-   ##
==========================================
+ Coverage   87.70%   87.76%   +0.06%     
==========================================
  Files           5        5              
  Lines         748      752       +4     
  Branches       97       98       +1     
==========================================
+ Hits          656      660       +4     
  Misses         56       56              
  Partials       36       36

☔ 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.

orbeckst · 2026-01-14T16:06:42Z

@PlethoraChutney thank you for your contribution and the insights. Could you please raise an issue that describes the issue — basically your justification for the fix? Separating solution from issues is good practice when we're discussing changes that may go against specs. Issues are our permanent record for decisions.

We can then link the PR to the issue.

Once you have the issue up, we can look closer at your proposed solution. Thank you!

orbeckst

Blocked until we have discussed the approach.

BradyAJohnston · 2026-01-14T16:43:33Z

It's related to this issue (BradyAJohnston/MolecularNodes#993) raised in MN. Downloading directly from the EMDB it would load fine, but if we round-trip through ChimeraX then it would fail to load with Grid() because of the points specified above.

Worth some proper discussion though!

orbeckst · 2026-01-14T16:45:57Z

Thanks for the context — ideally we collect all of this in an issue and then we have one place that documents the decision and the reasoning, as opposed to potentially mixing it with implementation details etc.

PlethoraChutney · 2026-01-14T17:26:31Z

Apologies! I'm relatively new to actually contributing to others' repos. Created #150 for discussion

orbeckst · 2026-01-14T17:43:22Z

Many thanks @PlethoraChutney , #150 is perfect.

Different projects have different "cultures" on how they do development and when coming in you just don't know.

PlethoraChutney · 2026-01-14T19:11:24Z

Tried to capture the discussion in #150

Grid() now takes a **kwargs parameter which is passed to all load functions. They all ignore it except MRC
MRC() takes load_mrc: None | bool. By default (None), mrcfile.is_volume() is used. If a bool is provided, it forces the file to be a volume (True) or a 2D image stack (False)
Added tests to cover this behavior

BradyAJohnston · 2026-01-14T19:30:21Z

gridData/core.py

    def __init__(self, grid=None, edges=None, origin=None, delta=None,
                 metadata=None, interpolation_spline_order=3,
-                 file_format=None):
+                 file_format=None, **kwargs):


I think I would prefer an actual argument rather than **kwargs as the latter can easily (and silently) absorb spelling mistakes fiel_format='MRC would be silently no longer passed on instead of raising an error otherwise. I defer to @orbeckst on this but I think just mentioning in the docs that it is only used for mrc files (and maybe a warning when used for non-compatible format?).

You're right that **kwargs can silently swallow typos. On the other hand, tracking individual modifier keywords for different parsers can get annoying, at least in principle. In MDAnalysis we've typically been using the approach of named keywords for arguments that universally apply and kwargs for modifiers.

GDF really does not see a flurry of development activity so the maintainability argument that I am raising is not particularly strong and I do see the value of being explicit and more robust for users. So then let's follow your idea and make is_volume a top-level kwarg of Grid, document it there and in MRC (with a proper versionadded), and propagate it explicitly.

I feel strongly about it mostly because I am endlessly spending hours bug fixing when it was just me with simple spelling mistakes.

orbeckst

Thank you for making the changes. Please see comments.

Please also

add an entry to CHANGELOG under Enhancements for 1.1.0
add yourself to AUTHORS
check that the docs render correctly (follow the link under the docs deployment check)

orbeckst · 2026-01-14T20:41:13Z

gridData/core.py

    def __init__(self, grid=None, edges=None, origin=None, delta=None,
                 metadata=None, interpolation_spline_order=3,
-                 file_format=None):
+                 file_format=None, **kwargs):


Per conversation: explicit is_volume=None kwarg.

Document it in the Grid docs with something like

is_volume : bool Indicate that the data to be loaded from a file is a density, even if the file may indicate differently. Currently only applies for MRC files (see :class:`gridData.MRC.MRC`) The default ``None`` indicates that the data is handled according to the file format specifications. .. versionadded:: 1.1.0

Or use the docs that you already wrote :-) (+ the versionadded)

orbeckst · 2026-01-14T20:41:32Z

gridData/core.py


            if filename is not None:
-                self.load(filename, file_format=file_format)
+                self.load(filename, file_format=file_format, **kwargs)


Per conversation: explicit is_volume=is_volume kwarg.

orbeckst · 2026-01-14T20:49:49Z

gridData/core.py

+    def _load_mrc(self, filename, **kwargs):
        """Initializes Grid from a MRC/CCP4 file."""
-        mrcfile = mrc.MRC(filename)
+        mrcfile = mrc.MRC(filename, **kwargs)


Explicitly pull out kwargs

Suggested change

mrcfile = mrc.MRC(filename, **kwargs)

mrcfile = mrc.MRC(filename, is_volume=kwargs.get("is_volume", None))

orbeckst · 2026-01-14T20:51:36Z

gridData/mrc.py

+       use the MRC file header. If True, force the file to be read as a
+       volume. If False, the file is intepreted as a 2D image stack and
+       raise a ValueError.
+        


Suggested change

.. versionadded:: 1.1.0

Check the rendered docs!

orbeckst · 2026-01-14T20:52:56Z

gridData/mrc.py

        self.filename = filename
        if filename is not None:
-            self.read(filename)
+            self.read(filename, is_volume)


Use kwargs explicitly, not implicitly.

Suggested change

self.read(filename, is_volume)

self.read(filename, is_volume=is_volume)

orbeckst · 2026-01-14T20:54:26Z

gridData/tests/test_mrc.py

+        Grid(datafiles.ISPG_0)
+
+def test_mrcfile_volume_force():
+    Grid(datafiles.ISPG_0, is_volume=True)


assert something about the data

PlethoraChutney · 2026-01-14T21:40:06Z

gridData/mrc.py

+            elif is_volume:
+                # non 3D volumes should always fail, regardless of is_volume value
+                is_volume = mrc.data is not None and len(mrc.data.shape) == 3
+            if not is_volume:                           #pragma: no cover


A question I had, never having used coverage.py before. Do we still need this pragma: no cover?

No, because you added an explicit test (which is great!).

Remove it. Good catch!

Better volumetric test

f391ee3

orbeckst requested changes Jan 14, 2026

View reviewed changes

orbeckst linked an issue Jan 14, 2026 that may be closed by this pull request

GridDataFormats refuses to load 3D MRC files with space group 0 #150

Open

orbeckst mentioned this pull request Jan 14, 2026

GridDataFormats refuses to load 3D MRC files with space group 0 #150

Open

MRC: force read as volume

a405729

BradyAJohnston reviewed Jan 14, 2026

View reviewed changes

orbeckst requested changes Jan 14, 2026

View reviewed changes

PlethoraChutney commented Jan 14, 2026

View reviewed changes

	mrcfile = mrc.MRC(filename, **kwargs)
	mrcfile = mrc.MRC(filename, is_volume=kwargs.get("is_volume", None))

	self.read(filename, is_volume)
	self.read(filename, is_volume=is_volume)

Better volumetric test #149

Are you sure you want to change the base?

Better volumetric test #149

Conversation

PlethoraChutney commented Jan 13, 2026

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

orbeckst commented Jan 14, 2026

Uh oh!

orbeckst left a comment

Choose a reason for hiding this comment

Uh oh!

BradyAJohnston commented Jan 14, 2026

Uh oh!

orbeckst commented Jan 14, 2026

Uh oh!

PlethoraChutney commented Jan 14, 2026

Uh oh!

orbeckst commented Jan 14, 2026

Uh oh!

PlethoraChutney commented Jan 14, 2026

Uh oh!

BradyAJohnston Jan 14, 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!

BradyAJohnston Jan 14, 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Jan 13, 2026 •

edited

Loading

BradyAJohnston Jan 14, 2026 •

edited

Loading

BradyAJohnston Jan 14, 2026 •

edited

Loading