JAL-2416: Load score matrices from file or resource file

Activity

CR-JAL-33 21

Keyboard shortcuts  
  • Summarize the review outcomes (optional)
     
    #permalink

    Details

    Warning: no files are visible, they have all been filtered.
    Participant Role Time Spent Comments Latest Comment
    Author 52m 11 Removed checks for aligned sequences for PCA. Also remove...
    Reviewer - Complete 2h 3m 10 I see on closer inspection that the DistanceScoreModel an...
    Total   2h 54m 21  
    #permalink

    Objectives

    Load/parse BLOSUM62, PAM250, DNA score matrices from resource files rather than hard-coding in ResidueProperties.
    Reuse the parser to allow drag and drop and/or menu option to load user score models.

    Branches in review

    #permalink

    Issues Raised From Comments

    Key Summary State Assignee
    #permalink

    General Comments

    Kira Mourão

    I think that the design of the score models class hierarchy could be slightly...

    I think that the design of the score models class hierarchy could be slightly simplified/improved. Currently SimilarityScoreModelI has a method findSimilarities and DistanceScoreModelI has findDistances. The implementations of these are (only) called by PCA::computeSimilarity and TreeBuilder::computeTree. In both cases they test for
    if (sm instanceof DistanceScoreModelI)
    {
    ... [call findDistances]
    }
    else if (sm instanceof SimilarityScoreModelI)
    {
    ... [call findSimilarities + slight adjustments which could be parameterised into the method call]
    }
    A better solution would be to have findDistances and findSimilarities as a single abstract method, implemented as per findDistances for the DistanceScoreModelI class and findSimilarities for the SimilarityScoreModelI classes. This would also remove the need for separate SimilarityScoreModelI and DistanceScoreModelI interfaces.

    Mungo Carstairs

    Not sure I quite get the plan. PCA and Tree do different tweaking of the scor...

    Not sure I quite get the plan. PCA and Tree do different tweaking of the score model outputs so need to 'know' whether they are similarity or distance scores. Need to discuss over a piece of paper.

    Kira Mourão

    I see on closer inspection that the DistanceScoreModel and SimilarityScoreMod...

    I see on closer inspection that the DistanceScoreModel and SimilarityScoreModel are tested opposite ways round in the PCA and TreeBuilder classes, but the same sort of thing applies. Have all the score model classes offer both findDistances and findSimilarities: the distance score model classes implement findSimilarities as findDistances with the reverseRange(false) call, and the similarity score model classes implement findDistances as findSimilarities with the reverseRange(true) call. Then PCA::computeSimilarity can replace the if statement with a call to sm.findSimilarities and TreeBuilder::computeTree can replace the if statement with a call to sm.findDistances with no need to use instanceof and downcasting. With the additional PIDModel case, the PIDModel implementation of findSimilarities returns the scaled result.
    The benefit of doing this is that PCA and TreeBuilder no longer need to know about the different types of models, new types of models can be added without changing the code, and the knowledge about the relationship between the similarity and distance scores is encapsulated in the score models code.

    Mungo Carstairs

    Done! As suggested, PIDModel.findSimilarities() performs the rescaling of pai...

    Done!
    As suggested, PIDModel.findSimilarities() performs the rescaling of pairwise identity scores from percentages to counts.
    This then has to be reversed in an override of findDistances(), to preserve the expected results for a Tree by PID.

    PCA.computeSimilarity() reduced to a single line
    scoreModel.findSimilarities();
    so just inlined this in the run() method.
    Deleted the (shortlived) PCATest, as it had just reduced to a test of findSimilarities, which is covered by other tests.

    /resources/lang/Messages.properties Changed
    /resources/lang/Messages_es.properties Changed
    /resources/scoreModel/blosum62.scm Changed
    Open in IDE #permalink
    /resources/scoreModel/blosum80.scm Changed
    Open in IDE #permalink
    /resources/scoreModel/dna.scm Changed
    Open in IDE #permalink
    /resources/scoreModel/pam250.scm Changed
    Open in IDE #permalink
    /resources/scoreModel/seqspace.scm Deleted
    Open in IDE #permalink
    /src/MCview/AppletPDBCanvas.java Changed
    /src/MCview/PDBCanvas.java Changed
    Open in IDE #permalink
    /src/.../scoremodels/FeatureDistanceModel.java Changed
    /src/jalview/.../scoremodels/PIDModel.java Changed
    /src/.../scoremodels/PIDScoreModel.java Deleted
    Open in IDE #permalink
    /src/.../scoremodels/PairwiseSeqScoreModel.java Deleted
    Open in IDE #permalink
    /src/jalview/.../scoremodels/ScoreMatrix.java Changed
    /src/jalview/.../scoremodels/ScoreModels.java Changed
    /src/.../scoremodels/SmithWatermanModel.java Changed
    /src/jalview/analysis/AlignSeq.java Changed 3
    /src/jalview/analysis/Conservation.java Changed
    /src/jalview/analysis/NJTree.java Changed 2
    /src/jalview/analysis/PCA.java Changed
    /src/jalview/api/analysis/ScoreModelI.java Changed
    /src/jalview/api/SiftsClientI.java Changed
    Open in IDE #permalink
    /src/jalview/appletgui/TreePanel.java Changed
    /src/jalview/datamodel/BinarySequence.java Changed
    /src/jalview/gui/CalculationChooser.java Changed 4
    /src/.../gui/ComboBoxTooltipRenderer.java Added
    Open in IDE #permalink
    /src/jalview/gui/PCAPanel.java Changed 3
    /src/jalview/gui/TreeChooser.java Deleted
    Open in IDE #permalink
    /src/jalview/gui/TreePanel.java Changed
    /src/jalview/io/IdentifyFile.java Changed
    /src/jalview/io/ScoreMatrixFile.java Changed
    /src/jalview/jbgui/GPCAPanel.java Changed 2
    /src/.../schemes/Blosum62ColourScheme.java Changed 3
    /src/.../schemes/ResidueProperties.java Changed
    /src/.../structure/StructureSelectionManager.java Changed
    /src/jalview/util/Comparison.java Changed
    /src/jalview/viewmodel/PCAModel.java Changed
    /src/jalview/ws/sifts/SiftsClient.java Changed
    /test/.../scoremodels/PIDModelTest.java Changed
    Open in IDE #permalink
    /test/.../scoremodels/ScoreMatrixTest.java Changed
    /test/.../scoremodels/ScoreModelsTest.java Added
    /test/jalview/analysis/AlignSeqTest.java Changed
    /test/jalview/analysis/TestAlignSeq.java Changed
    /test/jalview/io/IdentifyFileTest.java Changed
    Open in IDE #permalink
    /test/jalview/io/ScoreMatrixFileTest.java Changed
    /test/.../schemes/ScoreMatrixPrinter.java Changed

    Review updated: Reload | Ignore | Collapse

    You cannot reload the review while writing a comment.

    Log time against