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

Extend the supported data types in encoding alignment options #265

Conversation

matsutomo81
Copy link
Contributor

@matsutomo81 matsutomo81 commented Sep 9, 2022

The topic of this PR is about encoding SAM format into BAM format. According to BAM format specification, there are several data types for options in the alignment, but current cljam doesn't support all of them for encoding. If you try to encode a data type that is not supported, an Execution error will be returned.

This PR extends the data types of options that can be encoded, so that all data types except the H-type can be supported. Since the H-type seems to be a specification that remains only for backward compatibility, this PR did not provide support for the H-type encoding. Tests have also been added for the functions I have modified.

@codecov
Copy link

codecov bot commented Sep 9, 2022

Codecov Report

Merging #265 (8b5ca1e) into master (ccfae4e) will decrease coverage by 0.06%.
The diff coverage is 36.36%.

❗ Current head 8b5ca1e differs from pull request most recent head 33e5f1b. Consider uploading reports for the commit 33e5f1b to get more accurate results

@@            Coverage Diff             @@
##           master     #265      +/-   ##
==========================================
- Coverage   88.87%   88.80%   -0.07%     
==========================================
  Files          78       78              
  Lines        6389     6406      +17     
  Branches      442      453      +11     
==========================================
+ Hits         5678     5689      +11     
+ Misses        269      264       -5     
- Partials      442      453      +11     
Impacted Files Coverage Δ
src/cljam/io/bam/encoder.clj 67.03% <36.36%> (-8.65%) ⬇️
src/cljam/io/bam/decoder.clj 93.44% <0.00%> (+0.43%) ⬆️
src/cljam/io/util/lsb.clj 87.84% <0.00%> (+2.76%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@alumi alumi left a comment

Choose a reason for hiding this comment

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

Thank you for another PR @matsutomo81 👍 I added a comment about your concerns.
Since I just merged #264, please rebase and force-push to this branch when you finish editing the tests 🙏

@matsutomo81 matsutomo81 force-pushed the feature/Extend_the_supported_data_types_in_encoding_aln_options branch from 8b5ca1e to c47ebe3 Compare September 15, 2022 08:36
Copy link
Member

@alumi alumi left a comment

Choose a reason for hiding this comment

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

When you are done editing a PR, please click the "Re-request review" button on the reviewers list 🙏

test/cljam/io/bam/encoder_test.clj Show resolved Hide resolved
test/cljam/io/bam/encoder_test.clj Outdated Show resolved Hide resolved
test/cljam/io/bam_test.clj Outdated Show resolved Hide resolved
test/cljam/io/bam/encoder_test.clj Outdated Show resolved Hide resolved
test/cljam/io/bam_test.clj Outdated Show resolved Hide resolved
test/cljam/io/bam/encoder_test.clj Outdated Show resolved Hide resolved
test/cljam/io/bam_test.clj Outdated Show resolved Hide resolved
src/cljam/io/bam/encoder.clj Outdated Show resolved Hide resolved
src/cljam/io/bam/encoder.clj Outdated Show resolved Hide resolved
@matsutomo81 matsutomo81 force-pushed the feature/Extend_the_supported_data_types_in_encoding_aln_options branch from c47ebe3 to 33e5f1b Compare September 21, 2022 09:35
Copy link
Member

@alumi alumi left a comment

Choose a reason for hiding this comment

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

Thank you for updating the patch. Looks good 👍
It is recommended not to force-push to the PR branch while being reviewed on GitHub. This is meant for keeping track of inline review comments across commits. You can clean your commits up when all reviewers approve your PR if necessary.

@alumi alumi marked this pull request as ready for review September 26, 2022 02:34
@alumi alumi requested a review from a team as a code owner September 26, 2022 02:34
@alumi alumi requested review from xckitahara and removed request for a team September 26, 2022 02:34
Copy link
Contributor

@xckitahara xckitahara left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@xckitahara xckitahara merged commit 843e30b into chrovis:master Sep 28, 2022
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