Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Quaternary Symmetry Detection Broken with MMTF #517

Closed
lafita opened this issue Jun 17, 2016 · 12 comments
Closed

Quaternary Symmetry Detection Broken with MMTF #517

lafita opened this issue Jun 17, 2016 · 12 comments
Assignees
Labels
bug Bugs and bugfixes
Milestone

Comments

@lafita
Copy link
Member

lafita commented Jun 17, 2016

Using structures downloaded with the MMTF format break the QuatSymmetryDetector algorithm. It is related to the chemical component information. The tests explicitly set the format to MMCIF, so that is why the bug was not noticed when changing MMTF to default.

Here the stacktrace when the line cache.setUseMmCif(true); in TestQuatSymmetryDetection or DemoOrientBioAssembly is commented out (MMTF used):

java.lang.NullPointerException
    at org.biojava.nbio.structure.io.mmcif.ChemCompGroupFactory.getChemComp(ChemCompGroupFactory.java:47)
    at org.biojava.nbio.structure.HetatomImpl.getChemComp(HetatomImpl.java:429)
    at org.biojava.nbio.structure.ChainImpl.getSeqResSequence(ChainImpl.java:576)
    at org.biojava.nbio.structure.symmetry.core.ProteinChainExtractor.extractProteinChains(ProteinChainExtractor.java:121)
    at org.biojava.nbio.structure.symmetry.core.ProteinChainExtractor.run(ProteinChainExtractor.java:95)
    at org.biojava.nbio.structure.symmetry.core.ProteinChainExtractor.getCalphaTraces(ProteinChainExtractor.java:60)
    at org.biojava.nbio.structure.symmetry.core.ProteinSequenceClusterer.extractProteinChains(ProteinSequenceClusterer.java:98)
    at org.biojava.nbio.structure.symmetry.core.ProteinSequenceClusterer.run(ProteinSequenceClusterer.java:88)
    at org.biojava.nbio.structure.symmetry.core.ProteinSequenceClusterer.getSequenceAlignmentClusters(ProteinSequenceClusterer.java:56)
    at org.biojava.nbio.structure.symmetry.core.ClusterProteinChains.run(ClusterProteinChains.java:84)
    at org.biojava.nbio.structure.symmetry.core.ClusterProteinChains.(ClusterProteinChains.java:42)
    at org.biojava.nbio.structure.symmetry.core.QuatSymmetryDetector.run(QuatSymmetryDetector.java:104)
    at org.biojava.nbio.structure.symmetry.core.QuatSymmetryDetector.hasProteinSubunits(QuatSymmetryDetector.java:74)
    at org.biojava.nbio.structure.symmetry.analysis.CalcBioAssemblySymmetry.orient(CalcBioAssemblySymmetry.java:70)
    at demo.DemoOrientBioAssembly.runPDB(DemoOrientBioAssembly.java:108)
    at demo.DemoOrientBioAssembly.main(DemoOrientBioAssembly.java:81)
@lafita lafita added the bug Bugs and bugfixes label Jun 17, 2016
@abradle
Copy link
Contributor

abradle commented Jun 17, 2016

Hi Aleix!

Is this using the latest master? I think I found this bug (Wednesday) and fixed it Thursday.

@lafita
Copy link
Member Author

lafita commented Jun 17, 2016

As a side discussion related to this issue, should we make sure that tests do not change the defaults unless they are testing for a specific detail of the non-default options?

In this case, the format from which the structure is parsed should not make a difference in the symmetry detection. Otherwise, there is a problem with the parsing.

@lafita
Copy link
Member Author

lafita commented Jun 17, 2016

Thanks @abradle. I was unlucky and had not pulled your fix yet. Everything working fine now.

@abradle
Copy link
Contributor

abradle commented Jun 17, 2016

Aweseome! Yes I agree with that rule.

@abradle abradle closed this as completed Jun 17, 2016
@lafita
Copy link
Member Author

lafita commented Jun 17, 2016

I will change the tests to use the default (MMTF) to make sure future changes in the format work fine with this part.

@lafita
Copy link
Member Author

lafita commented Jun 20, 2016

I am not sure what changed since Friday, but TestQuatSymmetryDetection in the current master branch, commenting out the cache.setUseMmCif(true); line, throws the mentioned Exception again.

@lafita lafita reopened this Jun 20, 2016
@lafita lafita added this to the BioJava 5.0.0 milestone Jul 15, 2016
@abradle
Copy link
Contributor

abradle commented Jul 15, 2016

Sorry - forgot about this - thanks for assigning. Will look into it (seems to be to do with chem comps for seq res groups).

abradle added a commit to abradle/biojava that referenced this issue Jul 15, 2016
Fix the bug and clean up the logic of adding seq res groups
biojava#517
@lafita lafita closed this as completed Jul 18, 2016
@lafita lafita reopened this Jul 18, 2016
lafita added a commit that referenced this issue Jul 18, 2016
Some tests do not pass, issue #517 needs to be fixed.
@lafita
Copy link
Member Author

lafita commented Jul 18, 2016

@abradle pull request #538 does not fully fix this issue. I switched all tests of TestQuatSymmetryDetector back to mmtf and two of them fail. I pushed the commit to the master branch and it is failing in those tests.

As a hint, I think something of the chemical component information is not populated when generating the biological assemblies from symmetry mates and a NPE is thrown.

Thank you for looking at it and sorry to be insisting.

@josemduarte josemduarte assigned josemduarte and unassigned abradle Jul 25, 2016
@josemduarte
Copy link
Contributor

I've reassigned this one to me. I've been wanting to have a look at it for a while (it breaks the biojava build right now), but haven't got round to do it. I'll try in the next days.

@josemduarte
Copy link
Contributor

One additional issue I'm seeing right now: the tests outcome is different if run from maven than if they are run altogether in eclipse. That's due to issue #103 i.e. the singleton problem in AtomCache biting yet again.

For this particular case, what happens is that mmcif is used for some tests and then (if running in the same JVM) later tests reuse that state of the AtomCache without resetting it to mmtf.

@josemduarte
Copy link
Contributor

Correction: the singleton problem is in StructureIO, not AtomCache

@josemduarte
Copy link
Contributor

I tracked down the issue in TestQuatSymmetryDetection to a problem in the mmtf inflation where seqres groups are not populated correctly. They should at least have a PDBname (3-letter aminoacid code).

I've extended TestMmtfRoundTrip to test the seqres groups to demonstrate the issue, see this branch: https://github.com/josemduarte/biojava/tree/fixIssue517

josemduarte added a commit to josemduarte/biojava that referenced this issue Jul 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bugs and bugfixes
Projects
None yet
Development

No branches or pull requests

3 participants