-
Notifications
You must be signed in to change notification settings - Fork 397
Remove old mmCIF parsing and refactor chemcomp handling #912
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
Conversation
- replaced by DatabasePdbRevRecord
# Conflicts: # biojava-structure/src/test/java/org/biojava/nbio/structure/TestDownloadChemCompProvider.java
|
This is fantastic, thank you! I'll need some time to go through it in detail. Regarding the test We can then create a separate issue for the |
|
Just played around with the QCP algorithm. The test will find the correct solution if I 'trim' the precision of the double values coming from BinaryCIF. Otherwise the result is completely off. Kinda strange. Something along the lines of: |
josemduarte
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks great, big thank you!
I've added a few questions.
Also one missing thing is documenting the breaking changes. Could you add them to the CHANGELOG.md file so that when we release 6.0.0 it's already done?
biojava-integrationtest/src/test/java/org/biojava/nbio/structure/test/MMcifTest.java
Show resolved
Hide resolved
biojava-structure/src/main/java/org/biojava/nbio/structure/chem/ChemComp.java
Show resolved
Hide resolved
biojava-structure/src/main/java/org/biojava/nbio/structure/align/util/AtomCache.java
Show resolved
Hide resolved
biojava-structure/src/main/java/org/biojava/nbio/structure/align/util/AtomCache.java
Outdated
Show resolved
Hide resolved
Done in 53982b0. |
josemduarte
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the changes. I was going to say that the remaining issue is with test TestHardBioUnits#test4A1I, but strangely enough, it works now. There's something definitely weird in there. That deserves a separate issue to investigate the instabilities in SuperpositionQCP
|
Sounds good. I've opened an issue (#914). |
# Conflicts: # biojava-structure/src/test/java/org/biojava/nbio/structure/asa/TestAsaCalc.java
|
I'll merge this next week if there are no other comments. |
Would next week be a good time to discuss what might go in the version 6.0.0 release? There is a roadmap issue #879 |
|
I've reverted the changes to QCP (that would change the threshold). |
This PR completely replaces the old mmCIF parsing functionality with the ciftools-based implementation.
Major/breaking changes:
Bug fixes:
Known bugs/problems:
SCOP parsing requires MMTF for some reason (the corresponding tests would also fail with the old mmCIF parser)Let me know if you'd like to see things done differently somewhere.