Skip to content

fix off by one error in crosslink position calculation #456

Merged
NeleRiediger merged 3 commits into
crosslinkingfrom
453-off-by-one-error-in-crosslink-position-calculation
Jun 22, 2026
Merged

fix off by one error in crosslink position calculation #456
NeleRiediger merged 3 commits into
crosslinkingfrom
453-off-by-one-error-in-crosslink-position-calculation

Conversation

@NeleRiediger

@NeleRiediger NeleRiediger commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator

Description

fixes #453
Already explained in the Issue description.

Changes

  • I fixed the off-by-one error.
  • Then I fixed the tests that used 0 as a value for 1-based positions (those tests worked before because of the bug).
  • I also updated a docstring stating that the CL position within peptides was 0-based, while this is clearly not the case as stated elsewhere.
  • Lastly I also added a test making sure the amino acid of the CL position within peptides on the peptide sequence is the same as the amino acid of the absolute CL position on the whole protein sequence. So this would hopefully be caught in the future. (I had to move the function in question one layer up for this)

Testing

I described in the issue description how to reproduce the bug and you can check whether everything is correct now the same way.

PR checklist

Development

  • If necessary, I have updated the documentation (README, docstrings, etc.)
  • If necessary, I have created / updated tests.

Mergeability

  • main-branch has been merged into local branch to resolve conflicts
  • The tests and linter have passed AFTER local merge
  • The backend code has been formatted with black
  • The frontend code has been formatted with pnpm format and checked with pnpm lint

Code review

  • I have self-reviewed my code.
  • At least one other developer reviewed and approved the changes

@NeleRiediger NeleRiediger self-assigned this Jun 10, 2026
@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  backend/main
  views_helper.py 290-293
  backend/protzilla/data_analysis
  crosslinking_validation.py
  backend/protzilla/importing
  crosslinking_import.py
Project Total  

This report was generated by python-coverage-comment-action

@AnnaPolensky AnnaPolensky left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and I was able to validate the positions.

Small suggestion: Maybe we could include in the column names that the positions are 1-based.

@3dot141592

Copy link
Copy Markdown
Collaborator

I reproduced the issue you described in #453, and everything appears to be correct. Code looks perfect :).

@NeleRiediger

Copy link
Copy Markdown
Collaborator Author

Looks good and I was able to validate the positions.

Small suggestion: Maybe we could include in the column names that the positions are 1-based.

I adjusted the column names for "crosslinker_position1/2" to "1-based_crosslinker_position1/2" and "CL_position_within_peptide1/2" to "1_based_CL_position_within_peptide1/2". Maybe just sanity check if I messed anything up :)

@AnnaPolensky AnnaPolensky left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good :)

@NeleRiediger NeleRiediger merged commit 29360f9 into crosslinking Jun 22, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants