scastlara / minineedle

Needleman-Wunsch and Smith-Waterman algorithms in python
GNU General Public License v3.0
46 stars 8 forks source link

Library might not be working properly when debugging #19

Open Bornlex opened 4 months ago

Bornlex commented 4 months ago

When debugging using Pycharm, the objects in the current context are displayed in the debugger window, which calls their str method.

The thing is the str method itself calls the align method, filling the _alseq1 and _alseq2 attributes.

So when we call the self.align manually, both attributes are filled a second time, which creates a bug.

We could make sure the attributes are empty by assigning them to empty lists at the beginning of the _trace_back_alignment method:

def _trace_back_alignment(self, irow: int, jcol: int) -> None:

        # FIX
        self._alseq1, self._alseq2 = [], []
        # !FIX

        while True:
            if self._pmatrix[irow][jcol] == "diag":
                self._alseq1.append(self.seq1[jcol - 1])
                self._alseq2.append(self.seq2[irow - 1])
                if self.seq1[jcol - 1] == self.seq2[irow - 1]:
                    self._identity += 1
                irow -= 1
                jcol -= 1
            elif self._pmatrix[irow][jcol] == "up":
                self._alseq1.append(Gap(self._gap_character))
                self._alseq2.append(self.seq2[irow - 1])
                irow -= 1
            elif self._pmatrix[irow][jcol] == "left":
                self._alseq1.append(self.seq1[jcol - 1])
                self._alseq2.append(Gap(self._gap_character))
                jcol -= 1
            else:
                break
        self._alseq1 = list(reversed(self._alseq1))
        self._alseq2 = list(reversed(self._alseq2))
        self._identity = (self._identity / len(self._alseq1)) * 100
scastlara commented 4 months ago

Hey! Thank you for commenting. I've never used Pycharm, so unfortunately I've never encountered that issue.

In reality, what you found out is a fundamental issue with the design that I made many years ago when writing this. Ideally, the aligner instance should not hold the state of the alignment, but return an instance of some kind of alignment class. Having said that, I am not interested in changing anything about this: changing it will just annoy the odd user.

So your fix looks fine to me. Feel free to make a PR if you want!

Bornlex commented 4 months ago

Yes exactly. I don't think you want to perform the align operation when calling the str method, which should be pure rendering.

So maybe a better way of doing it would be to:

Would it work for you?

I'm going to create the PR.

Bornlex commented 4 months ago

I created a branch but I cannot push it. Can you please check the permissions?

scastlara commented 4 months ago

Agreed! You could create a fork and make a PR out of that no? Otherwise I will look it up in a while! I am afk at the moment!

Bornlex commented 4 months ago

Actually I cannot push the branch I created with the fix. I cannot create a PR if the branch is only local

scastlara commented 4 months ago

Actually I cannot push the branch I created with the fix. I cannot create a PR if the branch is only local

You would not be able to push to this repository. Instead you should create a fork of the repo, push a branch to it, and then create a pull request to this repository.

Here is a guide on how to do it: https://jarv.is/notes/how-to-pull-request-fork-github/