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

Update the alignment figure #62

Merged
merged 3 commits into from
Oct 1, 2022
Merged

Update the alignment figure #62

merged 3 commits into from
Oct 1, 2022

Conversation

alyst
Copy link
Contributor

@alyst alyst commented Nov 25, 2021

Update the alignment figure in the documentation

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)

  • ✨ 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

As a follow-up to #61, I have updated the figure. Now it combines both the pairwise alignment (which before was a verbatim text) and sequence of anchors (before it was Julia code, now it's a table in a SVG file). I think it makes the idea of the anchors more clear.

  • 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.

    • 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.

  • 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.

β˜‘οΈ Checklist

  • 🎨 The changes implemented is consistent with the julia style guide.
  • πŸ“˜ I have updated and added relevant docstrings, in a manner consistent with the documentation styleguide.
  • πŸ“˜ I have added or updated relevant user and developer manuals/documentation in docs/src/.
  • πŸ†— There are unit tests that cover the code changes I have made.
  • πŸ†— The unit tests cover my code changes AND they pass.
  • πŸ“ I have added an entry to the [UNRELEASED] section of the manually curated CHANGELOG.md file for this repository.
  • πŸ†— All changes should be compatible with the latest stable version of Julia.
  • πŸ’­ I have commented liberally for any complex pieces of internal code.

@codecov
Copy link

codecov bot commented Nov 25, 2021

Codecov Report

Merging #62 (2d3be18) into master (ed08737) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #62   +/-   ##
=======================================
  Coverage   87.95%   87.95%           
=======================================
  Files          16       16           
  Lines        1121     1121           
=======================================
  Hits          986      986           
  Misses        135      135           

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 ed08737...2d3be18. Read the comment docs.

Copy link
Member

@CiaranOMara CiaranOMara left a comment

Choose a reason for hiding this comment

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

Firstly, a belated thank you for putting together this PR.

I like the idea of an image because it can more comfortably display positions in the tens, something text images cannot do well. Including an image like the one you've constructed would be nice.

I appreciate that you may have moved to other things after all this time. If that's the case, please let us know so that someone else can step in and revive your PR.

AlignmentAnchor(19, 27, 26, OP_MATCH),
]
```
![Alignment representation](assets/alignment.svg)
Copy link
Member

Choose a reason for hiding this comment

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

I have got a few points regarding the image that I hope are trivial to address:

  • I think it is important for the dashes and dots in the alignment row to align with the query and reference.
  • Also, some of the position pointers/indicators appear between symbols. But, I think the symbols occupy the relative position/index, so I'd want these pointers to point to the middle of a symbol - please feel free to correct me.
  • I'm unconvinced that the enumeration in the circles adds information. So instead of enumerating the operations/anchors in the alignment row, could the aln position be shown instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • I think it is important for the dashes and dots in the alignment row to align with the query and reference.

I think it was the font problem. Now I have switched SVG to the embedded fonts, so the alignment should be ok. Let me know if some alignment issues still remain.

  • I'm unconvinced that the enumeration in the circles adds information. So instead of enumerating the operations/anchors in the alignment row, could the aln position be shown instead?

The numbers refer to the exact row in the table. If replaced by the alignment position (also then the alignment position should be moved to the right side of the block), matching operations on the diagram and in the table becomes less straightforward. So I would rather keep it as it is.

@MillironX MillironX merged commit f0b7689 into BioJulia:master Oct 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants