Added warning for 1 A^3 CRYST1 Record #2645

Oscuro-Phoenix · 2020-03-25T06:11:29Z

Fixes #2599

Changes made in this Pull Request:

Added a warning for 1 A^3 CRYST1 Record

PR Checklist

Tests?
Docs?
CHANGELOG updated?
Issue raised/referenced?

This reverts commit ee93e2e.

…into develop

This reverts commit 959d904.

Oscuro-Phoenix · 2020-03-25T06:12:25Z

Hi!,
Is this what we need? Do let me know if any changes are needed.
Thanks.

richardjgowers

Yeah this is where to start. Once we detect the box is 1,1,1 we want to reset it so size zero so that it doesn’t get used.

Then in addition to this we need a test which checks the code works

IAlibay

As per @richardjgowers' review, you'll probably want to skip feeding self.ts._unitcell if the CRYST record matches the cryo-em default pattern. I'd maybe consider restructuring this try/except block to include an else that validates the contents of the CRYST1 values before feeding self.ts._unitcells?

IAlibay · 2020-03-25T06:51:08Z

package/MDAnalysis/coordinates/PDB.py

@@ -398,6 +398,8 @@ def _read_frame(self, frame):
                    warnings.warn("Failed to read CRYST1 record, "
                                  "possibly invalid PDB file, got:\n{}"
                                  "".format(line))
+                if (self.ts._unitcell[:3] == np.array([1,1,1],dtype=float)).all():


For consideration: the Cryo-EM pattern is very specific to a 1,1,1,90,90,90 box. It might be overall safer to check for something that matches that explicitly?

Is this what we'd like?

try: cell_check = [line[6:15], line[15:24], line[24:33], line[33:40], line[40:47], line[47:54]] if (np.array(cell_check,dtype=float) == np.array([1,1,1,90,90,90],dtype=float)).all(): warnings.warn("1 A^3 CRYST1 record," "possibly an EM structure file, setting box-size to 0",stacklevel=2) self.ts._unitcell=np.array([0,0,0,90,90,90],dtype=float) except ValueError: warnings.warn("Failed to read CRYST1 record, " "possibly invalid PDB file, got:\n{}" "".format(line)) else: try: if not (self.ts._unitcell==np.array([0,0,0,90,90,90])).all(): self.ts._unitcell[:] = [line[6:15], line[15:24], line[24:33], line[33:40], line[40:47], line[47:54]] except ValueError: warnings.warn("Failed to read CRYST1 record, " "possibly invalid PDB file, got:\n{}" "".format(line))

Just a general point to start with. It is usually easier for us to review if you can push larger code changes to you branch rather than place it in the comments. That way we can get a better feeling for how the code changes fit within the context of the existing code. It also makes it easier for you to read our comments afterwards 😄

Whilst you have most of the right core elements, I don't think the structure works.

try block:

What you want to do here is try to extract the lines into a temporary variable (here cell_dims might be a more understandable variable name than cell_check). There's no point testing/warning for cryo-em default values here because you are already doing this in the else block (you don't want to use up extra CPU cycles if you can avoid it).

except block:

This seems fine.

else block:

The else block is where I'm having the hardest time following what you are doing.
Since you will have already extracted the cell dimensions in the try block, there is no point trying to extract them again. Additionally, you have already tested if a ValueError will be raised when reading the appropriate parts of line. So you don't need the addtiional try/except here.

In this part of the code I would expect you to be testing the contents of the temporary variable hosting the cell dimensions against the default cryo-em CRYST1 values (i.e. [1,1,1,90,90,90]). If it matches (i.e. is a likely cryo-em structure), then raise a warning. If not then feed the cell dimensions to self.ts._unitcell.

Thank you so much for taking this. I will keep your points in mind when I revert later.

What I wanted to do is basically have the cell_dims variable store the box values which are checked for CRYST1 values in the first try block. If they turn out to be the same as the specific set of values [1,1,1,90,90,90] then I set the self.ts._unitcell as having dimensions of [0,0,0,90,90,90]. And in else block, if the self.ts._unitcell is not set as having box-size of 0 then only I go ahead and set it as the cell_dims (again, I should have directly set it as =cell_dims). I see that the last try/except was extraneous. I'm really sorry for that.

package/MDAnalysis/coordinates/PDB.py

codecov · 2020-03-25T07:27:27Z

Codecov Report

Merging #2645 into develop will increase coverage by 0.15%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #2645      +/-   ##
===========================================
+ Coverage    90.89%   91.04%   +0.15%     
===========================================
  Files          174      159      -15     
  Lines        23592    21583    -2009     
  Branches      3084     3085       +1     
===========================================
- Hits         21443    19651    -1792     
+ Misses        1529     1314     -215     
+ Partials       620      618       -2

Impacted Files	Coverage Δ
package/MDAnalysis/coordinates/PDB.py	`90.17% <100.00%> (+0.08%)`	⬆️
...age/MDAnalysis/analysis/hbonds/hbond_autocorrel.py	`87.34% <0.00%> (-0.56%)`	⬇️
visualization/__init__.py
topology/__init__.py
formats/__init__.py
auxiliary/__init__.py
core/__init__.py
transformations/__init__.py
datafiles.py
__init__.py
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e01176f...bca4684. Read the comment docs.

This reverts commit c45f1ec.

IAlibay · 2020-03-26T08:06:56Z

@Oscuro-Phoenix not had a look at the code yet, but adding & parsing a 38k line (cryo-em files are indeed quite large 😅 ) file to test a CRYST1 record might a bit overkill. I'd instead recommend looking at using the much smaller 5A7U.

IAlibay

Some more changes in addition to my other #2645 (comment). Just a few more changes and we should be getting there 😃

testsuite/MDAnalysisTests/datafiles.py

testsuite/MDAnalysisTests/coordinates/test_pdb.py

IAlibay · 2020-03-26T11:11:07Z

testsuite/MDAnalysisTests/coordinates/test_pdb.py

+    #issue 2599  
+    with pytest.warns(UserWarning) as record:
+        u = mda.Universe(CRYST1_BOX)
+        cur_sele = u.select_atoms('around 0.1 (resid 4 and name CA and segid A)') 


You shouldn't need to call select_atoms to get the warning here (it should hopefully just be thrown on universe building. So the select_atoms shouldn't be necessary here. However, we definitely should be checking that selecting 0.1 A around one of the alpha carbons works now, so do please add this as a second test.

package/MDAnalysis/coordinates/PDB.py

IAlibay · 2020-03-26T11:20:16Z

package/MDAnalysis/coordinates/PDB.py

                except ValueError:
                    warnings.warn("Failed to read CRYST1 record, "
                                  "possibly invalid PDB file, got:\n{}"
                                  "".format(line))
-
+                else:
+                    if not (self.ts._unitcell==np.array([0,0,0,90,90,90])).all():


See above, I would be expecting most of what is currently in try (except assigning cell_dims) to be here.

Do let me know if it has been done the right way in my latest commit.

IAlibay

Few more comments @Oscuro-Phoenix

IAlibay · 2020-03-27T14:48:58Z

package/MDAnalysis/coordinates/PDB.py

+                    cell_dims = [line[6:15], line[15:24],
+                                  line[24:33], line[33:40],
+                                  line[40:47], line[47:54]]
+                    if (np.array(cell_dims, dtype=float) == 


As previously mentioned we don't want anything else but the assignment of cell_dims in the try block. Everything else needs to be in else.

IAlibay · 2020-03-27T15:33:27Z

package/MDAnalysis/coordinates/PDB.py

+                        np.array([1, 1, 1, 90, 90, 90], dtype=float)).all():
+                        warnings.warn("1 A^3 CRYST1 record," 
+                                      " possibly an EM structure file, setting box-size to 0",stacklevel=2)
+                        cell_dims = np.array([0, 0, 0, 90, 90, 90],dtype=float)    


If we match the CRYST1 record of a cryo-em file, we don't want to set the box at all. Setting cell_dims (and then eventually self.ts._unitcell[:] to [0, 0, 0, 90, 90, 90] technically means that some of the box was read in, which is not true here. Cryo-em CRYST1 records are just placeholder values so they don't have physicality.

IAlibay

Few more changes, nearly there :)

package/MDAnalysis/coordinates/PDB.py

testsuite/MDAnalysisTests/coordinates/test_pdb.py

package/MDAnalysis/coordinates/PDB.py

IAlibay

Nearly there, you'll also need to add a .. versionchanged:: 1.0.0 entry in the docstring of the PDBReader class.

package/MDAnalysis/coordinates/PDB.py

testsuite/MDAnalysisTests/coordinates/test_pdb.py

IAlibay · 2020-03-29T13:56:00Z

testsuite/MDAnalysisTests/coordinates/test_pdb.py

+
+def test_cryst_em_select():
+    #issue 2599
+    u=mda.Universe(PDB_CRYOEM_BOX)


PEP8, can you add spaces around = please? (one the lines below too).

testsuite/MDAnalysisTests/coordinates/test_pdb.py

IAlibay

Just the one final PEP8 change I think.

testsuite/MDAnalysisTests/coordinates/test_pdb.py

IAlibay

Looks good to me, just need to add an entry to CHANGELOG and add yourself to AUTHORS.
@richardjgowers any comments?

richardjgowers

LGTM

IAlibay

Somes changes for the CHANGELOG. Also you have some merge conflicts, you'll need to rebase against MDA's develop branch and solve the conflicts. If necessary, we can fix them for you on our end.

IAlibay · 2020-04-14T11:34:03Z

package/CHANGELOG

@@ -111,6 +111,7 @@ Enhancements
  * Improve the distance search in water bridge analysis with capped_distance (PR #2480)
  * Added weights_groupselections option in RMSD for mapping custom weights 
    to groupselections (PR #2610, Issue #2429)
+  * Added user warning for CRYST1 cryo-em structures (PR #2645, Issue #2599)


Two things here:

Since this does more than just add a warning I'd be more explicit here, something along the lines of "Checks for cryo-em 1 A^3 default CRYST1 record, disabling setting of box dimensions if found (Issue 1 A^3 boxes from EM structures in the PDB #2599)".

This probably should go under "fixes". Also as per the CHANGELOG rules, entries should be added "newest first", i.e. at the top of the list.

IAlibay

@Oscuro-Phoenix just the one addition needed to the CHANGELOG + the merge conflicts. Let me know if you are having issues rebasing, we can fix the merge conflicts on our end if needed.

package/CHANGELOG

IAlibay

Assuming Travis comes back green, LGTM! Thanks for all your work @Oscuro-Phoenix

IAlibay · 2020-04-15T18:25:07Z

Sorry about the close-open, just re-starting Travis in the hope that this MacOS build actually runs through...

Oscuro-Phoenix · 2020-04-15T20:31:29Z

Looks like there's some issue with Travis.

IAlibay · 2020-04-16T05:37:39Z

I'm going to ping @richardjgowers here, thoughts on the Travis failures? Different builds keep timing out. I guess cumulatively the whole thing came back green eventually. Just merge or try to fix Travis somehow?

richardjgowers · 2020-04-16T07:26:36Z

@Oscuro-Phoenix Thanks for the fix!

@IAlibay yeah travis is just a helper tool, you can merge stuff if you think you know better :)

Oscuro-Phoenix added 6 commits March 22, 2020 03:41

Fixed mdamath

c45f1ec

Added n_initial_contacts attribute

ee93e2e

Added n_initial_contacts

959d904

Revert "Added n_initial_contacts attribute"

e2fabab

This reverts commit ee93e2e.

Merge branch 'develop' of https://github.com/Oscuro-Phoenix/mdanalysis …

8bbdf55

…into develop

Revert "Added n_initial_contacts"

865f5c1

This reverts commit 959d904.

richardjgowers requested changes Mar 25, 2020

View reviewed changes

IAlibay requested changes Mar 25, 2020

View reviewed changes

Oscuro-Phoenix added 2 commits March 25, 2020 17:06

Revert "Fixed mdamath"

24776e4

This reverts commit c45f1ec.

Added Warning for 1A^3 CRYST1 Record

5a88586

Revised Warning CRYST1 and added test

db0e933

IAlibay requested changes Mar 26, 2020

View reviewed changes

IAlibay requested changes Mar 27, 2020

View reviewed changes

Oscuro-Phoenix added 2 commits March 28, 2020 01:03

Revised test for CRYST1

ee2124b

Resolved try

5be247b

IAlibay requested changes Mar 28, 2020

View reviewed changes

Oscuro-Phoenix added 3 commits March 29, 2020 04:54

Revised test 2

24647ac

Resolve dtype

0ddd0dc

PEP8

6b7b572

IAlibay requested changes Mar 29, 2020

View reviewed changes

testsuite/MDAnalysisTests/coordinates/test_pdb.py Outdated Show resolved Hide resolved

Oscuro-Phoenix added 3 commits March 30, 2020 01:40

Revised PEP8

a0fdd2b

Added docstring

febb227

PEP8 Fix

158f507

IAlibay requested changes Apr 14, 2020

View reviewed changes

richardjgowers approved these changes Apr 14, 2020

View reviewed changes

IAlibay requested changes Apr 14, 2020

View reviewed changes

Oscuro-Phoenix added 2 commits April 14, 2020 22:07

CHANGELOG and AUTHOR

579be07

CHANGELOG Update

7cb9179

IAlibay requested changes Apr 15, 2020

View reviewed changes

package/CHANGELOG Show resolved Hide resolved

Oscuro-Phoenix and others added 2 commits April 15, 2020 14:57

CHANGELOG Update 3

2bff8b1

Merge branch 'develop' into EM_cryst_fix

bca4684

IAlibay approved these changes Apr 15, 2020

View reviewed changes

Oscuro-Phoenix closed this Apr 15, 2020

Oscuro-Phoenix reopened this Apr 15, 2020

IAlibay closed this Apr 15, 2020

IAlibay reopened this Apr 15, 2020

IAlibay closed this Apr 16, 2020

IAlibay reopened this Apr 16, 2020

richardjgowers merged commit c35a6bf into MDAnalysis:develop Apr 16, 2020

orbeckst mentioned this pull request May 22, 2020

Fix #2679 #2685

Merged

4 tasks

fiona-naughton added defect Component-Readers labels Sep 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added warning for 1 A^3 CRYST1 Record #2645

Added warning for 1 A^3 CRYST1 Record #2645

Oscuro-Phoenix commented Mar 25, 2020 •

edited

Loading

Oscuro-Phoenix commented Mar 25, 2020

richardjgowers left a comment

IAlibay left a comment

IAlibay Mar 25, 2020

Oscuro-Phoenix Mar 25, 2020

IAlibay Mar 25, 2020

Oscuro-Phoenix Mar 26, 2020

codecov bot commented Mar 25, 2020 •

edited

Loading

IAlibay commented Mar 26, 2020

IAlibay left a comment

IAlibay Mar 26, 2020 •

edited

Loading

IAlibay Mar 26, 2020

Oscuro-Phoenix Mar 27, 2020

IAlibay left a comment

IAlibay Mar 27, 2020

IAlibay Mar 27, 2020

IAlibay left a comment

IAlibay left a comment

IAlibay Mar 29, 2020

IAlibay left a comment

IAlibay left a comment

richardjgowers left a comment

IAlibay left a comment

IAlibay Apr 14, 2020

IAlibay left a comment

IAlibay left a comment

IAlibay commented Apr 15, 2020

Oscuro-Phoenix commented Apr 15, 2020

IAlibay commented Apr 16, 2020

richardjgowers commented Apr 16, 2020

Added warning for 1 A^3 CRYST1 Record #2645

Added warning for 1 A^3 CRYST1 Record #2645

Conversation

Oscuro-Phoenix commented Mar 25, 2020 • edited Loading

PR Checklist

Oscuro-Phoenix commented Mar 25, 2020

richardjgowers left a comment

Choose a reason for hiding this comment

IAlibay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

try block:

except block:

else block:

Choose a reason for hiding this comment

codecov bot commented Mar 25, 2020 • edited Loading

Codecov Report

IAlibay commented Mar 26, 2020

IAlibay left a comment

Choose a reason for hiding this comment

IAlibay Mar 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IAlibay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IAlibay left a comment

Choose a reason for hiding this comment

IAlibay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IAlibay left a comment

Choose a reason for hiding this comment

IAlibay left a comment

Choose a reason for hiding this comment

richardjgowers left a comment

Choose a reason for hiding this comment

IAlibay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IAlibay left a comment

Choose a reason for hiding this comment

IAlibay left a comment

Choose a reason for hiding this comment

IAlibay commented Apr 15, 2020

Oscuro-Phoenix commented Apr 15, 2020

IAlibay commented Apr 16, 2020

richardjgowers commented Apr 16, 2020

Oscuro-Phoenix commented Mar 25, 2020 •

edited

Loading

`try` block:

`except` block:

`else` block:

codecov bot commented Mar 25, 2020 •

edited

Loading

IAlibay Mar 26, 2020 •

edited

Loading