Contact calculation performance tweaks - #1147
Conversation
aalhossary
left a comment
aalhossary
left a comment
There was a problem hiding this comment.
I recommend 2 small updates and raised another concern that may need a substantial change.
This is Amr (Not the AI 😊).
| } | ||
|
|
||
| public boolean hasContact(AtomIdentifier atomId1, AtomIdentifier atomId2) { | ||
| return contacts.containsKey(new Pair<AtomIdentifier>(atomId1,atomId2)); |
There was a problem hiding this comment.
Shouldn't the atom order be enforced here?
e.g.
if atom1 serial < atom2 serial then return hasContact(atomId2,atomId1)
There was a problem hiding this comment.
@josemduarte I checked Pair class hashCode() and confirmed that Pair(atom1, atom2).hashCode() != Pair(atom2, atom1).hashCode(), which is definitely correct.
However, for contact it should not be the case. We should update hasContact(), getContact() and getAtomIdPairFromContact()
Do you agree?
There was a problem hiding this comment.
I think you are right. But the issue is that changing this implementation can be a disruptive change for anywhere that assumes the current behaviour. I think what we can do right now is document it properly so no one trips over this. I've now added docs in e73ce56
There was a problem hiding this comment.
OK I'll create a new issue for this point, leave this thread unresolved, while approving the PR.
Hopefully I can find time (I doubt) to study the effect of the proposed update on other BioJava code and outer code.
Do you expect such behaviour change to qualify for BioJava 7.3.0 or 8.0.0?
Covers what is merged since 7.2.6 and what is open and expected to land: the download and checksum work, the CATH and ECOD fixes, the contact and ASA performance tweaks, electron density, and the JUnit 5 migration. Five entries are for pull requests that are still open - biojava#1134, biojava#1147, biojava#1148, biojava#1150 and biojava#1151 - and should be checked against what actually merged before the release is tagged.
aalhossary
left a comment
aalhossary
left a comment
There was a problem hiding this comment.
The PR provides useful updates.
However, reviewing it raised an issue (a potential bug), to be addressed later.
| } | ||
|
|
||
| public boolean hasContact(AtomIdentifier atomId1, AtomIdentifier atomId2) { | ||
| return contacts.containsKey(new Pair<AtomIdentifier>(atomId1,atomId2)); |
There was a problem hiding this comment.
OK I'll create a new issue for this point, leave this thread unresolved, while approving the PR.
Hopefully I can find time (I doubt) to study the effect of the proposed update on other BioJava code and outer code.
Do you expect such behaviour change to qualify for BioJava 7.3.0 or 8.0.0?
2 participants
A few performance tweaks that increase contact calculation performance ~ 1.5x