Skip to content
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

Allow chain id to be assigned with chainid!() #35

Merged
merged 3 commits into from
Oct 26, 2021

Conversation

gusennan
Copy link
Contributor

@gusennan gusennan commented Oct 21, 2021

Adds two function for assigning the chainid, chainid!(chain, new_id) and chainid!(residue, new_id).

For issue #22

A clear and descriptive title (No issue numbers please)

This template is rather extensive. Fill out all that you can, if are a new contributor or you're unsure about any section, leave it unchanged and a reviewer will help you 😄. This template is simply a tool to help everyone remember the BioJulia guidelines, if you feel anything in this template is not relevant, simply delete it.

Types of changes

This PR implements the following changes:
(Please tick any or all of the following that are applicable)

  • [ x] ✨ New feature (A non-breaking change which adds functionality).
  • 🐛 Bug fix (A non-breaking change, which fixes an issue).
  • 💥 Breaking change (fix or feature that would cause existing functionality to change).

📋 Additional detail

  • If you have implemented new features or behaviour

    • Provide a description of the addition in as many details as possible.

    • Provide justification of the addition.
      It's nice to be able to programmatically modify structures.

    • Provide a runnable example of use of your addition. This lets reviewers
      and others try out the feature before it is merged or makes it's way to release.
      I added some unit tests.

  • If you have changed current behaviour...

    • Describe the behaviour prior to you changes

    • Describe the behaviour after your changes and justify why you have made the changes,
      Please describe any breakages you anticipate as a result of these changes.

    • Does your change alter APIs or existing exposed methods/types?
      If so, this may cause dependency issues and breakages, so the maintainer
      will need to consider this when versioning the next release.

    • If you are implementing changes that are intended to increase performance, you
      should provide the results of a simple performance benchmark exercise
      demonstrating the improvement. Especially if the changes make code less legible.

Adds two function for assigning the chainid, chainid!(chain, new_id) and
chainid!(residue, new_id). For GitHub issue BioJulia#22.
@jgreener64
Copy link
Member

Thanks for trying this, I agree it is not easy.

What you have is a start but it is also important to update the key in the chains Dict of the Model that owns the Chain, otherwise mod[new_chainid] won't work.

I think there should be a new error type, something like PDBConsistencyError, which is thrown if you try to rename something to an already existing name (e.g. the new chain ID already exists in the model or the residue ID already exists in the chain it is moved to).

- Throw PDBConsistencyException when trying to change a chain's ID to an
ID that already exists.
- Throw PDBConsistencyException when moving a residue to a chain that
already has a residue with that number.
- Update the model's chain dictionary when changing a chain ID.
@gusennan
Copy link
Contributor Author

Thanks for the comments. I agree with your ideas. I've pushed a new commit I think addresses these shortcomings.

@codecov
Copy link

codecov bot commented Oct 26, 2021

Codecov Report

Merging #35 (7b84144) into master (d1fc832) will increase coverage by 0.06%.
The diff coverage is 100.00%.

❗ Current head 7b84144 differs from pull request most recent head c892e5a. Consider uploading reports for the commit c892e5a to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master      #35      +/-   ##
==========================================
+ Coverage   95.29%   95.36%   +0.06%     
==========================================
  Files           6        6              
  Lines        1553     1575      +22     
==========================================
+ Hits         1480     1502      +22     
  Misses         73       73              
Impacted Files Coverage Δ
src/model.jl 96.15% <100.00%> (+0.16%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d1fc832...c892e5a. Read the comment docs.

Copy link
Member

@jgreener64 jgreener64 left a comment

Choose a reason for hiding this comment

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

Thanks for making those changes. Just a few minor things and this is ready to merge.

src/model.jl Outdated Show resolved Hide resolved
src/model.jl Outdated Show resolved Hide resolved
test/runtests.jl Show resolved Hide resolved
src/model.jl Outdated Show resolved Hide resolved
- Added a test for ensuring KeyError thrown when ChainID changed to
something else.
@gusennan
Copy link
Contributor Author

@jgreener64 made those changes.

@jgreener64 jgreener64 merged commit 74a643a into BioJulia:master Oct 26, 2021
@jgreener64
Copy link
Member

Great, thanks for the contribution!

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.

2 participants