Better volumetric test

[PlethoraChutney]

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]

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]

@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]

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]

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]

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

[orbeckst]

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]

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]

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?).